-
Notifications
You must be signed in to change notification settings - Fork 33
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
Destroy component on multiple rerenders #189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
0d8672c to
8fe797d
Compare
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. 😁
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.
Uh oh!
There was an error while loading. Please reload this page.
This is a fix for #185
the problem was
rerendercreates anewComponent, but only ever checks for and destroys the originally renderedcomponent. Resolved this by reassigningcomponentwith the new, rerendered component so it always gets checked and destroyed when it should be.Also adds a test case for multiple rerenders.
Thanks!