1 Commits (remove-was-marked)

Author	SHA1	Message	Date
Eduardo Kurek	146cb5ea6c	fix: lazy props reactivity (#18146) ... Fix #18132 This PR treat lazy fallbacks on `prop()` as derived. Now a default function that uses a $state is recalculated whenever its dependents changes. This change implies that this lazy functions cannot mutate a state anymore (because it is derived), causing a `state_unsafe_mutation`error. This implies on a breaking change, but reasonable. --- ### New breaking change here - **Who does this affect**: Everyone that has updated a $state on a default lazy prop. Example: ```html <script> let myValue = $state(0); let callCount = $state(0); function getValue() { callCount++; // causes a state_unsafe_mutation error return myValue; // returning a state doesn't cause an error, and now it is tracked as a dependency } let { value = getValue() } = $props(); </script> ``` **Why make this breaking change** This encourages people to not update states on a function that fundamentaly, is readonly. When someone wants to use a default function expecting that it should be tracked, its not likely that this function will change some state. It is anti-pattern to change some state inside a getter function. But what if someone wants to do it, like in the code above? The code above doesn't make sense before this PR, the old way to calculate lazy functions is to execute it one time, and only one, so the `callCount` variable will never change. But let's assume that someone did it, how to migrate? The migration in same example is easy, since the `callCount` is executed only once, it will not be executed after the component is mounted. So the `callCount` doesn't need to be a state, the `callCount` will be in a valid state when the component is created. So here is the migrated code: ```html <script> let myValue = $state(0); let callCount = 0; function getValue() { callCount++; // doesn't causes an error return myValue; } let { value = getValue() } = $props(); </script> ``` As we can see, there is no reason for the variable `callCount` in this example (before this PR), and if someone did it, it is more likely that they used a constant instead: ```html <script> let myValue = $state(0); let callCount = 1; function getValue() { return myValue; } let { value = getValue() } = $props(); </script> ``` There is another example that causes the `state_unsafe_mutation` and how to fix (this happened on the tests that i changed): ```html <script> let log = $state([]); function fallbackExample => { log.push('fallback called'); return 1; // any value, just to show the issue with the log } let { value = fallbackExample() } = $props(); </script> ``` Here, we can see that `log` variable is a state. Before this PR, as i said, this function `fallbackExample` will be executed once. So the logs will be computed when the component is mounted. So there is no reason to make the `log` a state. The simplest way to fix this is to make it a normal variable: ```html <script> let log = []; </script> ``` But with this PR, the function might be recalculated at some point, and the `log` with a state makes sense now, so how to migrate in this case? As i said, changing a state inside a lazy prop function is not a good practice, we can think in a way to invert this dependency, and change the approach from push (imperative mutation) to pull (declarative derivation). If a developer really needs to track how many times a fallback is executed or react to its changes, they should use a $derived or an $effect that observes the same dependencies as the fallback, or simply observe the property itself: ```html <script> let { value = fallbackExample() } = $props(); let log = $state([]); $effect(() => { const message = `${value}`; untrack(() => { // we don't want to track changes on the log variable log.push(message); }); }); </script> ``` ### After all, how to migrate? 1. **If there is no state mutation inside the prop function, no need to changes**; 2. **If there is a state mutation inside the prop function, but the value muted is declared inside the same component:** remove the state from it. Before this PR the method will be executed only once, and to get the same result, you do not need the variable to be a state; **Before** ```html <script> let log = $state([]); const fallback_fn = () => { log.push('fallback_fn'); return 1; } const { myProp = fallback_fn() } = $props(); </script> ``` **After** ```html <script> let log = []; const fallback_fn = () => { log.push('fallback_fn'); return 1; } const { myProp = fallback_fn() } = $props(); </script> ``` 3. **If there is a state mutation inside the prop function, and the value is read in multiple places:** change your approach, use a effect to detect the change on the prop, and apply your mutation inside the effect; Before: ```html <script> import { setLog } from './logs.js'; // setLog apply a mutation on a state const fallback_fn = () => { setLog('fallback_fn'); return 1; } const { myProp = fallback_fn() } = $props(); </script> ``` After ```html <script> import { setLog } from './logs.js'; // setLog apply a mutation on a state const fallback_fn = () => { return 1; } const { myProp = fallback_fn() } = $props(); $effect(() => { const message = `${myProp}`; untrack(() => { setLog(message); }); }); </script> ``` ### Severity (number of people affected x effort): Low - **Affected Users:** Minimal. Mutating state inside a property initializer is a rare edge case and considered an anti-pattern (because its a side effect inside a getter). Most users use constants or pure functions for fallbacks. - **Migration Effort:** Low. As demonstrated in the examples above, the fix usually involves either removing an unnecessary $state or moving the side effect to its proper place, the $effect ### Conclusion This PR encourages users to program in a better way. Forcing a clean separation between data and their side effects. The developer can use this new feature mainly in i18n services, providing better usability and experience. Also, this PR makes the properties more predictable, since the expected behavior is that it works reactively, eliminating this bug for future developers. Even though this PR adds a breaking change, it's easily solvable, and the chance of any user facing this problem is low. **Full example to test reactivity in props** (won't work on web, you can get the PR and test localy to see it working): https://svelte.dev/playground/a6608434d8c642179f0e2b72468c74d7?version=latest *A unit test for this reactivity was created: runtime-runes/props-default-value-reactivity*. --------- Co-authored-by: Rich Harris <rich.harris@vercel.com> Co-authored-by: Rich Harris <hello@rich-harris.dev>	4 months ago