Skip to content
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

Merged
merged 6 commits into from
Aug 21, 2024
Merged

breaking: remove $state.link #12943

merged 6 commits into from
Aug 21, 2024

Conversation

Rich-Harris
Copy link
Member

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:

<script>
  let { data } = $props();

  let items = $state.link($state.snapshot(data.items));

  async function save() {
    // persist `items` to the db and invalidate `data`
  }
</script>

{#each items as item}
  <input bind:value={item.text} />
{/each}

<button onclick={save}>save</button>

Is that really so much better than this?

<script>
  let { data } = $props();

- let items = $state.link($state.snapshot(data.items));
+ let items = $state($state.snapshot(data.items));

+  $effect(() => {
+    items = $state.snapshot(data.items);
+  });

  async function save() {
    // persist `items` to the db and invalidate `data`
  }
</script>

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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Aug 20, 2024

🦋 Changeset detected

Latest commit: 420eb0d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@trueadm
Copy link
Contributor

trueadm commented Aug 20, 2024

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.

@Rich-Harris
Copy link
Member Author

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!

@ryanatkn
Copy link
Contributor

ryanatkn commented Aug 20, 2024

This REPL demonstrates an issue with $effect. I've run into issues $state.link would help with but I don't know what's good here.

@Rich-Harris
Copy link
Member Author

That kinda feels reasonable to me — the idea that save_data(items) should immediately reassign items to a different value would be surprising in any other context. I would typically expect a function with a name like save_data to do something like persist some data to the server and return what was persisted, meaning a) I would expect to use the return value, and b) I would expect the function to be async (demo):

-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 data.items works great:

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));

@ryanatkn
Copy link
Contributor

ryanatkn commented Aug 21, 2024

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.

@david-plugge
Copy link

david-plugge commented Aug 21, 2024

Is there a reason let items = $derived($state.snapshot(data.items)) would not work? To me that would make sense. A let derived seems to be what link should do.

@mimbrown
Copy link
Contributor

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.

@Hugos68
Copy link

Hugos68 commented Aug 21, 2024

Is there a reason let items = $derived($state.snapshot(data.items)) would not work? To me that would make sense. A let derived seems to be what link should do.

This is precisely what I was thinking, $state.link is in essence a read/write $derived but I think making $derived writable goes against it's name. If $derived becomes writable then it's no longer $derived, but rather $maybeDerived which probably won't be very good. That said a writable derived should exist imo, just under a different name 😄.

@sukeshpabolu
Copy link

I think we need a writable version of derived but it should be two way changeable.

@gterras
Copy link

gterras commented Aug 21, 2024

"Never assign state in effect except if you think it's necessary" will create thousands of horrific codebases.

@david-plugge
Copy link

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 dont see any point against using let x = $derived(...)

@Azarattum
Copy link
Contributor

I agree that we need a writable derived. It feels like a common enough use case for the framework to cover it. let ... $derived(... or maybe even $derived.mutable feel like a better API to me. Maybe it's just me, but I think it isn't obvious to tell what $state.link does just by looking at its name.

@Rich-Harris
Copy link
Member Author

there's a strong sense that setting state inside effects is always wrong/code smell

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:

It feels like a common enough use case for the framework to cover it

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.

Is there a reason let items = $derived($state.snapshot(data.items)) would not work?

There's a key difference between $derived and $state (or $state.link) which is that $derived doesn't wrap the resulting value in a proxy. So while items = ... (and the compiler took that reassignment to indicate that it should create a writable derived instead of a normal one) would update the UI, items.push(...) would not, because you'd be dealing with a vanilla JavaScript array.

So it would need to be something like $derived.mutable or $derived.writable, and it would need to have the same behaviour as $state.link (almost — if it was only mutated and never reassigned then I guess we could just proxy the result rather than create a separate source internally. But those are finicky implementation details that don't affect the API discussion).

@trueadm
Copy link
Contributor

trueadm commented Aug 21, 2024

I firmly believe that mutations to effects are bad, they cause so many problems in signal systems, including:

  • glitching, as now reading some state might capture the stale value
  • waterfalls and performance issues, if you update state in an effect, you're having to wait for the next microtask, which in turn will cascade state changes into the next microtask thereafter unless that effect is lucky enough to be scheduled after
  • future work, to enable forking/transactions in the future for things like error boundaries and suspense, we need to be able to avoid running effects in the work that is pending. If effects are used to synchronise state, then you'll have a pending fork/transaction that is incorrect
  • SSR hydration issues because effects aren't run on the server, you can work around these but it's so much harder to track down because things seem to work on the client, but they're not on the server

$state.link has some bugs right now with the second argument (my fault). I feel like we should land the PR that removes it and redesign a bug-free version that tackles the problems.

@gterras
Copy link

gterras commented Aug 21, 2024

I firmly believe that mutations to effects are bad

Why not disallowing them entirely, and provide solutions for the few legitimate use cases that arise? That would solve many issues at once:

  • would keep Svelte technical entry barrier low (as opposed to provide a shiny footgun that looks like it can solve many issues to the less experimented)
  • would remove the When not to use $effect part of the doc whose length kind of proves the point
  • would define a concrete list of legit cases and address them specifically in the docs (do this instead of don't do that)
  • would legitimate the $derived.mutable and others needed runes (as in avoid "just here to make your life easier" runes)
  • would consolidate Svelte5 as solution-provider framework instead of a be-careful framework

@mimbrown
Copy link
Contributor

I firmly believe that mutations to effects are bad, they cause so many problems in signal systems

For some reason I thought that Svelte 5 had a synchronous $effect.sync, but apparently it doesn't. Correct me if I'm wrong, but updating state in a synchronous effect would solve all but the last bullet, right? Is there a reason Svelte 5 doesn't yet have this?

<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>

@Rich-Harris
Copy link
Member Author

Why not disallowing them entirely, and provide solutions for the few legitimate use cases that arise?

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 Promise.resolve() to hide their sins from the Correctness Police, and no one will be better off

@Rich-Harris
Copy link
Member Author

Is there a reason Svelte 5 doesn't yet have this?

Tearing. If the effect depends on both a and b, and you do [a, b] = ..., the effect will run twice, and the first time only a will be correct. In many cases that's fine, but when it breaks it breaks in very subtle ways (for example array[n] = ... might also update array.length, and having the length be inconsistent with the contents could cause real problems).

@trueadm
Copy link
Contributor

trueadm commented Aug 21, 2024

For some reason I thought that Svelte 5 had a synchronous $effect.sync, but apparently it doesn't. Correct me if I'm wrong, but updating state in a synchronous effect would solve all but the last bullet, right? Is there a reason Svelte 5 doesn't yet have this?

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
  }
}

@mimbrown
Copy link
Contributor

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 $state.subtle namespace?

@gterras
Copy link

gterras commented Aug 21, 2024

Why not disallowing them entirely, and provide solutions for the few legitimate use cases that arise?

A regrettable lack of omniscience on our part - we just don't know, exhaustively, what those legitimate use cases are

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.

People will start to do terrible things

The point is with current implementation they already do.

@Rich-Harris
Copy link
Member Author

A less invasive approach would be to issue a warning on writes-inside-effects that can be squelched with a svelte-ignore comment, much as we have ignorable warnings for mutations of objects owned by parent components. That would push people towards better outcomes without bolting the escape hatch shut.

@trueadm
Copy link
Contributor

trueadm commented Aug 21, 2024

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 $state.subtle namespace?

I'm saying that $effect.sync causes more issues than it solves. I can assure you that it can suffer from this having seen the same thing happen – we actually used to have a sync effect internally within Svelte and it causes all manner of mayhem on performance with signal invalidation (if you invoke a sync effect and that invalidates the parent effect, then you can run into issues where the reactive signal graph is broken).

@Rich-Harris
Copy link
Member Author

Regardless of where we end up, it's clear that there are a lot of unresolved questions — even if we were to agree that some API is needed, there's a reasonable argument for replacing $state.link with $derived.writable or something completely different.

I really think we should revert #12545 ASAP to limit the damage caused by API churn, and revisit it once we've had the opportunity to properly stress-test a few designs.

@mimbrown
Copy link
Contributor

With Svelte 5's lazy signal evaluation system, sync effects aren't possible. You can always force all effects to be sync with flushSync, but having part of graph work like this won't work.

Interesting. I think of the invalidation of the value of a $derived as itself a sync effect, which is what makes me think that there has to be some mechanism for this that works, which is why I'm having a hard time letting this go I guess.

@Rich-Harris
Copy link
Member Author

Deriveds are invalidated synchronously, but evaluated lazily (aka 'push-pull')

@mimbrown
Copy link
Contributor

Right, but that invalidation is still "code that runs synchronously when a signal's value changes", which is exactly what $effect.sync would be. It's just that users can write code inside the $effect.sync that does very bad things.

@Rich-Harris
Copy link
Member Author

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.

@mimbrown
Copy link
Contributor

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 mark_reactions(source, DIRTY), that function is a synchronous effect triggered by the change to the signal, even if you don't call it an effect.

running user code during that phase would unleash Zalgo

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.

@mattcroat
Copy link

mattcroat commented Aug 21, 2024

Regardless of where we end up, it's clear that there are a lot of unresolved questions — even if we were to agree that some API is needed, there's a reasonable argument for replacing $state.link with $derived.writable or something completely different.

Yes, please! It makes so much more sense to have $derived.writable.

Not only does it feel odd to use $state to create derived values, and I've read some reasons why making a derived writable isn't actually a derived, but who cares? To be honest I was excited for $state.link, but after trying it out I don't think that's the way, and I'd rather have nothing instead.

People use $effect out of convenience, when they could use an event listener, or an object with a get, and set method to create a writable derived — which is more intuitive compared to trying to wrap your head around $state.link to be honest.

Please give it a Viking funeral. 🫡

@trueadm
Copy link
Contributor

trueadm commented Aug 21, 2024

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 mark_reactions(source, DIRTY), that function is a synchronous effect triggered by the change to the signal, even if you don't call it an effect.

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 .

@Rich-Harris
Copy link
Member Author

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?

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. something is currently impossible to do
  2. something is possible, but cumbersome
  3. something is possible, but the outcome is suboptimal

(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.

future work, to enable forking/transactions in the future for things like error boundaries and suspense

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).

@mimbrown
Copy link
Contributor

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 $state.link was meant to deliver, and then it became about sync effects (irrelevant here).

@Rich-Harris
Copy link
Member Author

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.

@Rich-Harris Rich-Harris merged commit 9f962d6 into main Aug 21, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the revert-12545-state-link branch August 21, 2024 19:29
@mimbrown mimbrown mentioned this pull request Aug 21, 2024
7 tasks
@Hugos68
Copy link

Hugos68 commented Aug 21, 2024

Yes, please! It makes so much more sense to have $derived.writable.

How would this work though, will we have:
$derived.by
$derived
$derived.by.writable
$derived.writable

?

I much rather have
$derived.by
$derived

And have const or let make it readable and writable

@Azarattum
Copy link
Contributor

Azarattum commented Aug 22, 2024

May I also suggest $derived.fork as a shorter alternative for writable or mutable. It makes sense that we are forking the state, so that changes are independent but still sync with "upstream" if that makes sense. That would also preserve the original derived term semantics a bit better

Though I still like the let/const idea more (if only, of course, it would be technically reasonable to implement)

@KieranP
Copy link

KieranP commented Aug 22, 2024

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)

@aradalvand
Copy link

aradalvand commented Aug 22, 2024

Why isn't $derived just mutable by default? (genuine question)

@Hugos68
Copy link

Hugos68 commented Aug 22, 2024

Why isn't $derived just mutable by default? (genuine question)

Because it it's mutable it no longer is derived anymore, but since $derived.mutable was mentioned by Rich himself I'm starting to think the semantic name is not as important as I thought it was.

@ryanatkn
Copy link
Contributor

ryanatkn commented Aug 23, 2024

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 items without possibly getting stale values. Anything deferred a tick is fine, like rendering, and it's easily worked around if you know what's happening. An obvious improvement is to write helpers that accept arguments instead of using the local closure scope, but this feels like an error-prone corner case, and a synchronously-consistent design would avoid the problem.

This lets us think more in terms of "just the signal" (pulling) instead of reasoning about when things update.

@Rich-Harris
Copy link
Member Author

But is helper_called_here_and_elsewhere operating on data.items — the source of truth — or items, the local copy? If it's being invoked after save_data then I'd expect it to be operating on the source of truth, which is up to date after save_data returns.

Why isn't $derived just mutable by default? (genuine question)

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 $derived(todos.filter((t) => !t.done)) will edit the original todos, not a copy. This is by design. But we don't add reactivity where it didn't previously exist, because deriveds are classically thought of as being read-only.

If we made deriveds mutable by default (using the same semantics as $state, i.e. POJOs are proxified but everything else, including existing state proxies, are left untouched) then most of the time we'd be adding overhead for no good reason, because most of the time they are meant to be read-only (and any mutations will be obliterated when new stuff comes in anyway). So there needs to be some mechanism for deciding which derived values are proxified:

  • it can't be let, because many people reject const on principle, and in any case that's just not what the distinction between let and const means
  • it can't be the fact that a derived is reassigned, because you might want to mutate a derived that never is reassigned
  • it can't be the fact that a derived is mutated, because you can't reliably determine that (the mutation could happen somewhere the compiler can't 'see')
  • so it would need to be an explicit opt-in thing like $derived.mutable

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.

@ryanatkn
Copy link
Contributor

ryanatkn commented Aug 23, 2024

But is helper_called_here_and_elsewhere operating on data.items — the source of truth — or items, the local copy?

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.

@Azarattum
Copy link
Contributor

Azarattum commented Aug 23, 2024

it can't be let, because many people reject const on principle, and in any case that's just not what the distinction between let and const means

@Rich-Harris, I feel like all the feedback I've seen here is people rooting for const/let and not rejecting the const. In the plain JS you can mutate const, but not reassign it which is exactly the same behavior that you have described with read-only derived. I feel like it makes total sense here. Also with const/let you'd get type checking for free, as TS wouldn't let you reassign a const, unlike with the .writable approach where you'd have to check for it in the Svelte's linter.

If you meant that people reject const as a code style choice in general, I wouldn't say that it is a strong argument against it. In Svelte 4 the derived state was declared with $: which is far less vanilla than just a plain const and people were fine with it.

@Rich-Harris
Copy link
Member Author

I mean in JavaScript more generally — it's a very common sentiment that const was a design mistake since constants are mutable, and many people insist on only ever using let. We absolutely cannot make the let/const distinction load-bearing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.