Incorrect prop value in onDestroy when there's a setter but not getter in the template
#16262
Development PRs
Fixes: #16114
When a $state or a $derived is directly set by the user, we properly add it to the old_values Map.
However when a derived is updated as a result of the check_dirtiness check during flush_queued_effects, we do not save the old value, hence the value read in a teardown function of a $effect is not the old value but the new one, this is incosistent with the "old value behavior" that was recently introduced.
In this PR i save the old value when the $derived is updated, so that it will be used by teardown functions of $effects.
However while working on this i've noticed that there's an edge case when working with child components.
While passing a prop in certain conditions (either a legacy bindable or a prop accessed via $.prop because it has a setter somewhere in the code) the original $state is wrapped in a derived. When the child component is destroyed the teardown functions might read $.prop values that did not got update because thay were not read (perhaps in the template). This means that is possible that during the execution of the teardown the values of the derived are either null because they were never initialized because nothing ever read from it, or they are outdated. I've opened a separate issue for this: #16262.
This bug is present both before and after the changes of this PR.
Before submitting the PR, please make sure you do the following
- It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- Prefix your PR title with
feat:,fix:,chore:, ordocs:. - This message body should clearly illustrate what problems it solves.
- Ideally, include a test that fails without this PR but passes with it.
- If this PR changes code within
packages/svelte/src, add a changeset (npx changeset).
Tests and linting
- Run the tests with
pnpm testand lint the project withpnpm lint
alternative to #16263, made possible by #16270. Closes #16114, closes #16262
Before submitting the PR, please make sure you do the following
- It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- Prefix your PR title with
feat:,fix:,chore:, ordocs:. - This message body should clearly illustrate what problems it solves.
- Ideally, include a test that fails without this PR but passes with it.
- If this PR changes code within
packages/svelte/src, add a changeset (npx changeset).
Tests and linting
- Run the tests with
pnpm testand lint the project withpnpm lint
Issue
Describe the bug
When a prop has a setter (for instance <button onclick={() => (checked = !checked)}></button>) but it doesn't have a getter (like <p>{checked}</p>), the value read during onDestroy does not match either the old or the current value of the prop, it's instead read as null.
This happens because the compiler decides to use $.prop to read the prop since it detects a "setter", $.prop "wraps" the prop in a derived but since we never read from it, when we execute onDestroy the derived is still in an "uninitialized" state hence it returns null instead of the correct value.
Reproduction
https://svelte.dev/playground/4fa745c18a3c41e88bda761f37b36783?version=5.34.9
Click count++ twice, the 2nd time the Component is unmounted and onDestroy is called and it will log the incorrect checked value that should be true but is logged as null.
System Info
System:
OS: Windows 11 10.0.26100
CPU: (32) x64 Intel(R) Core(TM) i9-14900HX
Memory: 41.79 GB / 63.74 GB
Binaries:
Node: 23.5.0 - C:\Program Files\nodejs\node.EXE
Yarn: 1.22.22 - C:\Program Files\nodejs\yarn.CMD
npm: 11.0.0 - C:\Program Files\nodejs\npm.CMD
pnpm: 10.4.0 - ~\AppData\Local\pnpm\pnpm.CMD
Browsers:
Edge: Chromium (137.0.3296.93)
Internet Explorer: 11.0.26100.1882Severity
annoyance