Skip to content

breaking: state mutations inside the template are no longer allowed #13660

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

Merged
merged 7 commits into from
Oct 19, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-chairs-build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

breaking: state mutations inside the template are no longer allowed
Original file line number Diff line number Diff line change
Expand Up @@ -125,5 +125,5 @@ Reading state that was created inside the same derived is forbidden. Consider us
### state_unsafe_mutation

```
Updating state inside a derived or logic block expression is forbidden. If the value should not be reactive, declare it without `$state`
Updating state inside a derived or a template expression is forbidden. If the value should not be reactive, declare it without `$state`
```
2 changes: 1 addition & 1 deletion packages/svelte/messages/client-errors/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,4 @@

## state_unsafe_mutation

> Updating state inside a derived or logic block expression is forbidden. If the value should not be reactive, declare it without `$state`
> Updating state inside a derived or a template expression is forbidden. If the value should not be reactive, declare it without `$state`
4 changes: 2 additions & 2 deletions packages/svelte/src/internal/client/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,12 +343,12 @@ export function state_unsafe_local_read() {
}

/**
* Updating state inside a derived or logic block expression is forbidden. If the value should not be reactive, declare it without `$state`
* Updating state inside a derived or a template expression is forbidden. If the value should not be reactive, declare it without `$state`
* @returns {never}
*/
export function state_unsafe_mutation() {
if (DEV) {
const error = new Error(`state_unsafe_mutation\nUpdating state inside a derived or logic block expression is forbidden. If the value should not be reactive, declare it without \`$state\``);
const error = new Error(`state_unsafe_mutation\nUpdating state inside a derived or a template expression is forbidden. If the value should not be reactive, declare it without \`$state\``);

error.name = 'Svelte error';
throw error;
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/reactivity/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ export function template_effect(fn) {
value: '{expression}'
});
}
return render_effect(fn);
return block(fn);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, can we just change the type of effect? I would've expected this has unintended side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's pretty much the same thing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what ways are they not? If template_effect which is pretty much used for all things that are output into the html isn't using a render effect, then what do we even need it for?

Also did you see my comment about only doing the validation in runes mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation was always runes mode anyway, we're re-using the same validation as before. Blocks differ in two ways:

  • They don't clear destroy branch effects on updates
  • They disallow mutations as do deriveds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked differently: what's stopping us from keep using the render effect, if we would extract the validation into a common method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately $effect.pre and a bunch of other stuff already using render effect with mutations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this change affect or be affected in some by way the work done in #13600 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they're connected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @dummdidumm on this — treating every template expression as a block effect seems like it could have undesirable consequences, if not now then in the future. For example it means that in update_effect we do destroy_block_effect_children instead of destroy_effect_children, which means we don't destroy child deriveds, which feels like it could be bad.

Shouldn't we just have a new flag that means 'state mutations not allowed' and apply that to deriveds and blocks and template expressions?

Then in set we'd do something like this

-(active_reaction.f & (DERIVED | BLOCK_EFFECT)) !== 0 &&
+(active_reaction.f & (MUTATIONS_DISALLOWED)) !== 0 &&

Copy link
Contributor Author

@trueadm trueadm Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means we don't destroy child deriveds, which feels like it could be bad.

That's an oversight, we should always be destroying them. #13693 fixes that so they're now consistent.

Shouldn't we just have a new flag that means 'state mutations not allowed' and apply that to deriveds and blocks and template expressions?

I'm hesitant adding more flag conditions, we already have a ton and each one we add removes from our total allowed, which removes future additions for features around async etc. However maybe we can do that – I just felt it was unnecessary to introduce a flag when the two common concepts – deriveds and block effects have the same unified heuristic.

}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
compileOptions: {
dev: true
},

test({ assert, target }) {
const button = target.querySelector('button');

assert.throws(() => {
button?.click();
flushSync();
}, /state_unsafe_mutation/);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let items = $state([]);
</script>

<button onclick={() => items.push(3, 2, 1)}>Add</button>
{JSON.stringify(items.sort())}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<script>
import { untrack } from "svelte";
import { writable, derived } from "svelte/store";

const obj = writable({ a: 1 });
Expand All @@ -7,7 +8,9 @@

function watch (prop) {
return derived(obj, (o) => {
count++;
untrack(() => {
count++;
});
return o[prop];
});
}
Expand Down