-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
breaking: remove $state.link
#12943
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
breaking: remove $state.link
#12943
Conversation
🦋 Changeset detectedLatest commit: 420eb0d 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 |
Effects aren’t the solution here. They are glitchy so reading from them between mutations like you can deriveds doesn’t work. SSR mismatches happen too as the effect doesn’t run on the server. This rune is very much needed. |
No-one has been able to explain to me why it's needed, short of an example in #12545 that was honestly a bit too complicated for me to follow. Give me concrete examples! |
This REPL demonstrates an issue with |
That kinda feels reasonable to me — the idea that -save_data(items);
+const saved = await save_data(items);
-// This is reading a stale value:
-console.log('`items` is stale after `save_data` because of the `$effect` delay:', $state.snapshot(items));
+// This is reading a current value:
+console.log('`items` is current after `save_data`:', $state.snapshot(saved)); Aside from that, it's worth noting that referencing save_data(items);
-// This is reading a stale value:
-console.log('`items` is stale after `save_data` because of the `$effect` delay:', $state.snapshot(items));
+// This is reading a current value:
+console.log('`data.items` is current after `save_data`:', $state.snapshot(data.items)); |
That makes sense to me, it seems like a corner case that only slightly breaks my consistency expectations that Svelte 5 has set. It's also setting state in an effect which I side-eye, but I'll be thinking about when that's actually okay. It's also easy to use a helper to avoid the wasteful effect on mount. IME it just highlights that particular pattern and isn't a problem, though it is a little convoluted because the effect still needs to read any signals it depends on. |
Is there a reason |
Worth noting: from what I've seen on the Discord server, there's a strong sense that setting state inside effects is always wrong/code smell. If someone other than you recommended the code that you wrote up top, they would get pushback. There may need to be some debunking/education from the top around when it's ok to do that. |
This is precisely what I was thinking, |
I think we need a writable version of derived but it should be two way changeable. |
"Never assign state in effect except if you think it's necessary" will create thousands of horrific codebases. |
Deriving something from something does not mean the derived thing can not be edited imo. That can easily be specified by the user depending in wether let or const is used. Updating an object in place already works today. |
I agree that we need a writable derived. It feels like a common enough use case for the framework to cover it. |
It's definitely something that's best avoided (I've banged this drum plenty myself), but occasionally needs do arise. Though the 'occasionally' brings me back to this:
I'd still love to get a better sense of what these use cases are. In my experience it's been a very uncommon use case — uncommon enough that bending the 'rules' feels preferable to adding new API surface area.
There's a key difference between So it would need to be something like |
I firmly believe that mutations to effects are bad, they cause so many problems in signal systems, including:
|
Why not disallowing them entirely, and provide solutions for the few legitimate use cases that arise? That would solve many issues at once:
|
For some reason I thought that Svelte 5 had a synchronous <script>
let { data } = $props();
let items = $state($state.snapshot(data.items));
- $effect(() => {
+ $effect.sync(() => {
items = $state.snapshot(data.items);
});
async function save() {
// persist `items` to the db and invalidate `data`
}
</script> |
A regrettable lack of omniscience on our part - we just don't know, exhaustively, what those legitimate use cases are. There are always going to be times when people need an escape hatch to do something naughty in the service of shipping, and if we put barriers in the way for ideological reasons it will make the framework annoying to use. People will start to do terrible things like putting the mutation behind a |
Tearing. If the effect depends on both |
The only thing it would solve is glitching with stale values, you will still have subtle issues though as Rich points out above. There would still be waterfalls, unless the entire app used sync effects and effects still won't run in async forks. However, sync effects have their own set of problems: let arr = $state([]);
$effect.sync(() => {
if (arr.length > 0) {
// do something expensive
}
});
function add_items() {
for (let i = 0; i < 10000; i++) {
arr.push(something); // this will now invoke the above effect 1000 times
}
} |
I boil your two responses in my mind down to, "we don't have it because it would be easy to misuse." But this is the place where it would be nice to have, I don't think using a sync effect to update one state when another changes would suffer from any of the problems you just mentioned. Does there need to be a |
Then what if there is none? Most of the current rewrite design comes from user feedback anyway. Just let it be the same but knowingly this time.
The point is with current implementation they already do. |
A less invasive approach would be to issue a warning on writes-inside-effects that can be squelched with a |
I'm saying that |
3fdae0d
to
a7999ab
Compare
Deriveds are invalidated synchronously, but evaluated lazily (aka 'push-pull') |
Right, but that invalidation is still "code that runs synchronously when a signal's value changes", which is exactly what |
Nothing runs when the derived is invalidated, we're just making a note of which functions will need to run. As Dominic explains, running user code during that phase would unleash Zalgo. Today, Zalgo mighͭ̉t̢͈̩ s̘ͯț̻͐ḁr̼ͤͥt̜͊ w̸͈͂ͫ̕ͅh̜̋̎̔͋̂is͕͓̞͎̯̍͑̽́́͢͞p̗̼̥͖͆́́̓͘e̵̛̲̾̐́̄̅̅r̥̀͐in̛̮̲̏͆ͭ̀̋̊͢͟͝g̨̛̛̮͕͚͎̐̀̅͊̅̓͜͢ to you when you write to state inside effects, but he remains leashed. |
Your javascript internals are not nothing, they are javascript functions same as ours, just (rightly) privileged to have access to the internals. What I'm saying is that when you call
That's fair, except I would say could instead of would. My question is, what is the path forward to allow for responsible library authors to write the complex, tricky code necessary to extend the Svelte 5 primitives and enable the higher-order use cases? I feel like that is the missing piece right now. |
Yes, please! It makes so much more sense to have Not only does it feel odd to use People use Please give it a Viking funeral. 🫡 |
Unfortunately, that won't work. If we ran an effect sync here then the graph would break if the effect had its own dependencies. It gets even trickier as running it here wouldn't even be correct – we have to run effects in order of the component tree to ensure parents effects run in sequence with child effects. Furthermore, because of unowned/disconnected deriveds skip this propagation of marking dirty, it might not even encounter the sync effect. We instead use versioning here and rely on the lazy pull re-connecting the effects to the graph later. It's all very complicated for a good reason – this is stuff that very few libraries get right and is also why we're collaborating on the TC39 signals proposal . |
It really depends on what those use cases are. The discussion has been frustratingly abstract so far. There's basically three reasons you might add an API:
(1) is pretty rare as long as you have escape hatches, and I don't think it applies here. When you're dealing with (2) and (3) you're into the realm of trade-offs — does the cumbersomeness or suboptimality outweigh the cost of new API? And that's a very hard question to answer without having real world examples to hold in your hand. Personally I'm less concerned about glitches and waterfalls — I think you have to work pretty hard for them to be actually problematic as opposed to theoretical, and if we limit the blast radius (by warning when you assign to state inside effects, to discourage it) then it's probably fine most of the time.
We do want to be mindful of future plans, but to me this is a strong argument for being more conservative in what APIs we add, rather than adding APIs that might mitigate some hypothetical future harms that we don't fully understand yet (and only for the subset of people who choose to use them rather than doing the obvious thing with effects). |
I helped derail this, I apologize. The original point was that I believed a synchronous effect would potentially be a better workaround to achieve the same behavior that |
I'm going to go ahead and merge this PR, because the longer the API remains shipped the more harm will come from people adding it to their codebases, and the less likely it is that we'll be able to make improvements (such as renaming it) due to inertia bias. Apologies for creating this situation. |
How would this work though, will we have: ? I much rather have And have |
May I also suggest Though I still like the |
Why could we not allow $state to act like $derived, and return a derived value wrapped in a proxy? This is ideally what I personally would like to do: let something = $state(1)
let something_doubled = $state(something * 2)
// something_doubled == 2 (1 x 2)
something += 1
// something_doubled == 4 (2 x 2)
something_doubled += 1
// something_doubled == 5 (previous value + 1)
something += 1
// something_doubled == 6 (3 x 2) |
Why isn't |
Because it it's mutable it no longer is derived anymore, but since |
re: @Rich-Harris Hopefully this is helpful as a related feature gets redesigned/punted - consider that you may have a function that expects to be able to do this: save_data(items);
helper_called_here_and_elsewhere(); The helper can't reference This lets us think more in terms of "just the signal" (pulling) instead of reasoning about when things update. |
But is
Do you mean mutable or writable? Those are two related but different things. Mutable would mean that the return value is proxified, so that property assignments trigger updates, whereas writable means you can assign a whole new value to the derived (which persists until the input value changes). Deriveds can in fact be mutable today... // you can mutate `d.count`, it won't cause the derived to re-evaluate
let d = $derived.by(() => {
let object = $state({ count: 0 });
return object;
}); ...and they will preserve the reactivity of their inputs — editing members of If we made deriveds mutable by default (using the same semantics as
If deriveds were writable but not mutable, you'd run into weird behaviour with things like this: let a = $state({ count: 0 });
let b = $derived(a); // mutating `b.count` also mutates `a.count`, as intended
function temporarily_overwrite_b(new_count) {
b = { new_count }; // oops! this is just a POJO, mutating `b.count` now has no effect
} (Nor do I think a runtime heuristic like 'proxify the new value if the old one happened to be a proxy' is particularly robust.) I think there's something worth exploring in these ideas but I also think we should take our time to design it right, rather than trying to rush it to get it into 5.0 (or worse, delaying 5.0 because of it). In the meantime, effects are a perfectly decent escape hatch, I think. |
I guess I don't want to have to think about it, and I want to sloppily read any state in scope, and have it be synchronously consistent in the graph. The hypotheticals have easy fixes, I'm advocating for the value of keeping as much as possible in that synchronous pullable context, so people won't have the opportunity to run into these corner cases. My 2 cents is I haven't run into enough situations where I think this is important for 5.0 due to the workarounds, but I agree the set of primitives feels incomplete. |
@Rich-Harris, I feel like all the feedback I've seen here is people rooting for If you meant that people reject |
I mean in JavaScript more generally — it's a very common sentiment that |
Is there a bigger reason why this isn't the solution? The word "derived" signifies a relationship of origin, it doesn't explicity say it can't evolve or change. And declaring a derived with let vs const is clean, simple and familiar. |
Does anyone really think |
This reverts #12545, as a more invasive alternative to #12942.
Here's the rationale: if we merge #12942, then what exactly is left of the API? Basically just this:
Is that really so much better than this?
In this scenario, the precise timing really doesn't matter. So the argument against it is that it's slightly more verbose code. But in exchange, we reduce surface area, and we have a place to put more complex logic (like merging incoming changes with local state, etc).
I think we should remove
$state.link
, and I'm annoyed at myself for not reaching this conclusion earlier.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint