Merge pull request #8164 from sashkashishka/bugfix/cleantree-stops-on-first-slot-in-dom-render-fn-cleanup

fix: execute cleanup cb for all component tree while calling dispose.cleanup method returned by render fn

Author: wmertens

.changeset/bumpy-cycles-carry.md

@@ -0,0 +1,9 @@
+---
+'eslint-plugin-qwik': patch
+'create-qwik': patch
+'@builder.io/qwik-react': patch
+'@builder.io/qwik-city': patch
+'@builder.io/qwik': patch
+---
+
+execute cleanup cb for all component tree while calling dispose.cleanup method returned by render fn

packages/qwik/src/core/render/dom/render.public.ts

@@ -124,7 +124,7 @@ export const injectQContainer = (containerEl: Element) => {
 
 function cleanupContainer(renderCtx: RenderContext, container: Element) {
   const subsManager = renderCtx.$static$.$containerState$.$subsManager$;
-  cleanupTree(container, renderCtx.$static$, subsManager, true);
+  cleanupTree(container, renderCtx.$static$, subsManager, true, true);
 
   removeContainerState(container);
 

packages/qwik/src/core/render/dom/render.unit.tsx

@@ -778,14 +778,37 @@ test('should clean up subscriptions after calling the returned cleanup function'
   const fixture = new ElementFixture();
 
   const spies = {
-    cleanupSpy: false,
+    cleanup: false,
   };
 
   const { cleanup } = await render(fixture.host, <CleanupComponent spies={spies} />);
 
   cleanup();
 
-  assert.equal(spies.cleanupSpy, true);
+  assert.equal(spies.cleanup, true);
+});
+
+test('should clean up nested subscriptions after calling the returned cleanup function', async () => {
+  const fixture = new ElementFixture();
+
+  const spies = {
+    parentCleanup: false,
+    cleanup: false,
+    slottedCleanup: false,
+  };
+
+  const { cleanup } = await render(
+    fixture.host,
+    <ParentCleanupComponent spies={spies}>
+      <SlottedCleanupComponent spies={spies} />
+    </ParentCleanupComponent>
+  );
+
+  cleanup();
+
+  assert.equal(spies.parentCleanup, true);
+  assert.equal(spies.cleanup, true);
+  assert.equal(spies.slottedCleanup, true);
 });
 
 async function expectRendered(fixture: ElementFixture, expected: string) {
@@ -1045,16 +1068,56 @@ export const Hooks = component$(() => {
 
 //////////////////////////////////////////////////////////////////////////////////////////
 
-export const CleanupComponent = component$((props: { spies: { cleanupSpy: boolean } }) => {
+interface CleanupProps {
+  spies: {
+    parentCleanup?: boolean;
+    cleanup?: boolean;
+    slottedCleanup?: boolean;
+  };
+}
+
+export const ParentCleanupComponent = component$((props: CleanupProps) => {
   useTask$(({ cleanup }) => {
     cleanup(() => {
-      props.spies.cleanupSpy = true;
+      props.spies.parentCleanup = true;
+    });
+  });
+
+  return (
+    <div>
+      <div id="parent-cleanup">true</div>
+      <CleanupComponent spies={props.spies}>
+        <Slot />
+      </CleanupComponent>
+    </div>
+  );
+});
+
+export const CleanupComponent = component$((props: CleanupProps) => {
+  useTask$(({ cleanup }) => {
+    cleanup(() => {
+      props.spies.cleanup = true;
     });
   });
 
   return (
     <div>
       <div id="cleanup">true</div>
+      <Slot />
+    </div>
+  );
+});
+
+export const SlottedCleanupComponent = component$((props: CleanupProps) => {
+  useTask$(({ cleanup }) => {
+    cleanup(() => {
+      props.spies.slottedCleanup = true;
+    });
+  });
+
+  return (
+    <div>
+      <div id="slotted-cleanup">true</div>
     </div>
   );
 });

packages/qwik/src/core/render/dom/visitor.ts

@@ -1135,11 +1135,12 @@ export const cleanupTree = (
   elm: Node | VirtualElement,
   staticCtx: RenderStaticContext,
   subsManager: SubscriptionManager,
-  stopSlots: boolean
+  stopSlots: boolean,
+  dispose = false
 ) => {
   subsManager.$clearSub$(elm);
   if (isQwikElement(elm)) {
-    if (stopSlots && elm.hasAttribute(QSlotS)) {
+    if (!dispose && stopSlots && elm.hasAttribute(QSlotS)) {
       staticCtx.$rmSlots$.push(elm);
       return;
     }
@@ -1150,7 +1151,7 @@ export const cleanupTree = (
     const end = isVirtualElement(elm) ? elm.close : null;
     let node: Node | null | VirtualElement = elm.firstChild;
     while ((node = processVirtualNodes(node))) {
-      cleanupTree(node!, staticCtx, subsManager, true);
+      cleanupTree(node!, staticCtx, subsManager, true, dispose);
       node = node.nextSibling;
       if (node === end) {
         break;

starters/apps/starter-partytown-test/src/root.tsx

@@ -9,7 +9,12 @@ export default () => {
       <head>
         <meta charset="utf-8" />
         <title>Qwik + Partytown Blank App</title>
-        <script dangerouslySetInnerHTML={partytownSnippet({ debug: true })} />
+        <script
+          dangerouslySetInnerHTML={partytownSnippet({
+            fallbackTimeout: 1000,
+            debug: true,
+          })}
+        />
       </head>
       <body>
         <App />

starters/e2e/e2e.attributes.spec.ts

@@ -302,11 +302,13 @@ test.describe("attributes", () => {
   test.describe("client rerender", () => {
     test.beforeEach(async ({ page }) => {
       const toggleRender = page.locator("#force-rerender");
+      const renderCount = page.locator("#renderCount");
       const v = Number(await toggleRender.getAttribute("data-v"));
+
+      expect(v).toBe(0);
+      await expect(renderCount).toHaveText(`Render ${v}`);
       await toggleRender.click();
-      await expect(page.locator("#renderCount")).toHaveText(`Render ${v}`);
-      // Without this the tests still fail?
-      await page.waitForTimeout(50);
+      await expect(renderCount).toHaveText(`Render ${v + 1}`);
     });
     tests();
   });

starters/e2e/e2e.context.spec.ts

@@ -187,7 +187,6 @@ test.describe("context", () => {
     test.beforeEach(async ({ page }) => {
       const rerender = page.locator("#btn-rerender");
       await rerender.click();
-      await page.waitForTimeout(100);
     });
     tests();
   });

starters/e2e/e2e.style.spec.ts

@@ -17,9 +17,13 @@ test.describe("styles", () => {
   test.describe("client side", () => {
     test.beforeEach(async ({ page }) => {
       const reload = page.locator("#reload");
+      const renderCount = page.locator("#renderCount");
       const v = Number(await reload.getAttribute("v"));
+
+      expect(v).toBe(0);
+      await expect(renderCount).toHaveText(`Render ${v}`);
       await reload.click();
-      await expect(page.locator("#renderCount")).toHaveText(`Render ${v}`);
+      await expect(renderCount).toHaveText(`Render ${v + 1}`);
     });
     runTests();
   });

starters/e2e/qwikcity/nav.spec.ts

@@ -59,8 +59,6 @@ test.describe("actions", () => {
 
         const link = page.locator("#hash-1");
         await link.click();
-        // Without this, sometimes the URL is #hash-1
-        await page.waitForTimeout(100);
 
         await expect(page).toHaveURL(
           "/qwikcity-test/scroll-restoration/hash/#hash-2",

starters/playwright.config.ts

@@ -19,6 +19,9 @@ expect.extend({
 
 const config: PlaywrightTestConfig = {
   use: {
+    launchOptions: {
+      slowMo: 100,
+    },
     viewport: {
       width: 520,
       height: 600,
@@ -27,8 +30,9 @@ const config: PlaywrightTestConfig = {
   /* Fail the build on CI if you accidentally left test.only in the source code. */
   forbidOnly: !!process.env.CI,
   testIgnore: /.*example.spec.tsx?$/,
-  retries: inGithubCI ? 0 : 1,
-  expect: { timeout: inGithubCI ? 120000 : 10000 },
+  retries: 0,
+  // retries: inGithubCI ? 0 : 1,
+  expect: { timeout: inGithubCI ? 120000 : 3000 },
   webServer: {
     command:
       "pnpm node --require ./scripts/runBefore.ts starters/dev-server.ts 3301",