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

[sashkashishka]

What is it?

• Bug

Description

We have a use case when we render a qwik container client side only (while SPA navigation). We faced with the problem, when leaving the page and calling dispose function, previously rendered components don't call cleanup function in useTasks at all. After some investigation we found that it only affects child components rendered in a Slot. So everything above the slot calls cleanup, but below it doesn't.

Here is an example:

const Child = component$(() => {
  useVisibleTask$(async ({ cleanup }) => {
    console.log('mount Child');
    cleanup(() => console.log('unmount Child')); // 3
  });

  return (
    <div style={{ background: 'red', color: 'white' }}>
      Child component
    </div>
  );
});

const Provider = component$(() => {
  useTask$(({ cleanup }) => {
    console.log('mount Provider');
    cleanup(() => console.log('unmount Provider')); // 2
  });

  return (
    <Slot />
  );
});


const Root = component$(() => {
  useTask$(({ cleanup }) => {
    console.log('mount root');
    cleanup(() => console.log('unmount')); // 1
  });

  return (
    <Provider>
      <Child />
    </Provider>
  );
});

render(document.getElementById('root'), <Root />)

So when we dispose the component, it will only call 1 and 2 cleanup function, but not 3.

But actually, it should call all three cleanups.

Checklist

• My code follows the developer guidelines of this project
• I performed a self-review of my own code
• I added a changeset with pnpm change
• I made corresponding changes to the Qwik docs
• I added new tests to cover the fix / functionality

Besided, on my local machine I faced with the failing e2e tests (it turned out that my laptop is too fast and test assertions was conducted before the js manages to render the content). Please, check out if the provided fixes works for you. if not, I'll revert them. Also, found some bugs with rerender test cases and fixed them (there were assertions to check whether the counter was incremented, but it hasn't managed to do that and the assertion looked like expect(0).toBe(0) instead of checking whether the increment has worked expect(1).toBe(1). you can find it here

[changeset-bot]

🦋 Changeset detected

Latest commit: 36d4b7e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
eslint-plugin-qwik	Patch
create-qwik	Patch
@builder.io/qwik-react	Patch
@builder.io/qwik-city	Patch
@builder.io/qwik	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@builder.io/qwik@8164

npm i https://pkg.pr.new/@builder.io/qwik-city@8164

npm i https://pkg.pr.new/eslint-plugin-qwik@8164

npm i https://pkg.pr.new/create-qwik@8164

commit: 36d4b7e

[github-actions]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name	Status	Preview	Last Commit
qwik-docs	✅ Ready (View Log)	Visit Preview	36d4b7e

[wmertens]

[wmertens]

@sashkashishka very nice! Can you check if this problem also exists in v2?

[gioboa]

Thanks for your help @sashkashishka
Some packages are not affected by this change though. 🤔

[sashkashishka]

only eslint? looks like these one are using qwik as dependency. I'll open a new PR with fix

'create-qwik': patch
'@builder.io/qwik-react': patch
'@builder.io/qwik-city': patch
'@builder.io/qwik': patch

[sashkashishka]

@sashkashishka very nice! Can you check if this problem also exists in v2?

sure, but at the end of the week