fix: store access on component destroy #14968
Merged
+96
−16
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #14950
While working on this initially with @oscard0m i actually found out of a separate bug, a memory leak: basically if you did something like this
the store was accessed for the first time after the teardown of the stores executed. So a new subscription was created for the store that generally would've been cleaned up by the teardown but since it already ran it was stuck in memory.
This was even worse for stuff that was accessed on component destroy asynchronously.
This solution does two things:
setup_stores
function doesn't register theteardown
immediately but returns a function that does that. This function is called afterpop
so that register even after every user effect.stores
variable as a way of communication...it register a symbol on it so that insideget_store
we can check if the component was already unmounted and, in that case, it prevents the creation of a subscription in the store while at the same time accessing the store withget
fromsvelte/store
, less efficient but more consistent (the bug we are closing was about the consistency of the read) and not leaky. Furthermore this will only really happen when someone is accessing stuff after the component unmounts so should not be a problem.I didn't found a way to test the memory leak but if you have ideas it could be good to add.
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint