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>
pull/18163/head
Eduardo Kurek 3 months ago committed by GitHub
parent bc82a55647
commit 146cb5ea6c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: re-run fallback props if dependencies update

@ -1,4 +1,4 @@
/** @import { Effect, Source } from './types.js' */
/** @import { Derived, Effect, Source } from './types.js' */
import { DEV } from 'esm-env';
import {
PROPS_IS_BINDABLE,
@ -283,8 +283,14 @@ export function prop(props, key, flags, fallback) {
var fallback_value = /** @type {V} */ (fallback);
var fallback_dirty = true;
var fallback_signal = /** @type {Derived<V> | undefined} */ (undefined);
var get_fallback = () => {
if (lazy && runes) {
fallback_signal ??= derived(/** @type {() => V} */ (fallback));
return get(fallback_signal);
}
if (fallback_dirty) {
fallback_dirty = false;

@ -1,5 +1,5 @@
<script>
let log = $state([]);
let log = [];
const fallback_value = 1;
const nested = {

@ -1,5 +1,5 @@
<script>
let log = $state([]);
let log = [];
const fallback_value = 1;
const nested = {

@ -0,0 +1,19 @@
import { flushSync, tick } from 'svelte';
import { test } from '../../test';
export default test({
accessors: false,
test({ assert, target }) {
const btn = target.querySelector('button');
btn?.click();
flushSync();
assert.htmlEqual(
target.innerHTML,
`
<p>greeting: Hola</p>
<button>Change Language</button>
`
);
}
});

@ -0,0 +1,10 @@
<script>
import Sub from './sub.svelte';
import { set_translation } from './translations.svelte.js';
</script>
<Sub />
<button onclick={() => set_translation('Hola')}>
Change Language
</button>

@ -0,0 +1,9 @@
<script>
import { get_translation } from './translations.svelte.js';
const {
p0 = get_translation()
} = $props();
</script>
<p>greeting: {p0}</p>

@ -0,0 +1,9 @@
let greeting = $state('Hello');
export function get_translation() {
return greeting;
}
export function set_translation(value) {
greeting = value;
}
Loading…
Cancel
Save