Skip to content

Destroy component on multiple rerenders #189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

jgbowser
Copy link
Contributor

@jgbowser jgbowser commented Apr 5, 2022

This is a fix for #185

the problem was rerender creates a newComponent, but only ever checks for and destroys the originally rendered component. Resolved this by reassigning component with the new, rerendered component so it always gets checked and destroyed when it should be.

Also adds a test case for multiple rerenders.

Thanks!

@yanick yanick self-requested a review April 5, 2022 17:28
Copy link
Collaborator

@yanick yanick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jgbowser jgbowser force-pushed the destroy-component-on-multiple-rerenders branch from 0d8672c to 8fe797d Compare April 5, 2022 17:37
@yanick
Copy link
Collaborator

yanick commented Apr 7, 2022

I'll be wild and assume that since no-one said anything so far, that's it's good and mashing that 'merge' button is totally okay. 😁

@yanick yanick merged commit a6cc764 into testing-library:main Apr 7, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

🎉 This PR is included in version 3.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@basarbk
Copy link
Contributor

basarbk commented Nov 20, 2022

I believe this is not a correct approach to fix this. Rerender does not necessearily require component to be destroyed.
Lets say there is a UserPage and it is visible for paths like /user/:id . A sample routing would be like this (either with svelte-routing or svelte-navigator). We can pass the id as param

<Route path="/user/:id" let:params>
   <UserPage id={params.id} />
</Route>

When we navigate from /user/1 to /user/2, UserPage component is not destroyed. Only the prop, id is now has the value, 2.

But in test, since rerender is destroying and mounting the component, even an incorrect implementation like this one is passing the test. So it ends up with false positive result. But in reality, the same scenario fails.

I have this repo shows the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants