breaking: state mutations inside the template are no longer allowed #13660
Conversation
🦋 Changeset detectedLatest commit: 48ee7bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -325,7 +325,7 @@ export function template_effect(fn) { | |||
| value: '{expression}' | |||
| }); | |||
| } | |||
| return render_effect(fn); | |||
| return block(fn); | |||
There was a problem hiding this comment.
I'm confused, can we just change the type of effect? I would've expected this has unintended side effects.
There was a problem hiding this comment.
There's pretty much the same thing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Asked differently: what's stopping us from keep using the render effect, if we would extract the validation into a common method?
There was a problem hiding this comment.
Unfortunately $effect.pre and a bunch of other stuff already using render effect with mutations.
There was a problem hiding this comment.
Can this change affect or be affected in some by way the work done in #13600 ?
There was a problem hiding this comment.
I don't think they're connected.
There was a problem hiding this comment.
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 &&There was a problem hiding this comment.
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.
|
#13684 makes me worried to go ahead with this so close to release - feels like there's hidden bugs and the cure could be worse than the disease |
That was unrelated and has since been fixed. :) |
A logical extension of #13625. Given we don't allow mutations in template logic block expressions, it also makes sense to no longer allow the inside template expressions either.
Closes #13633
Fixes #13677