fix: invoke $state.link callback at the correct time

[Rich-Harris]

The current implementation involves some juggling that I don't really understand, and invokes the callback on read rather than just whenever the incoming value changes. It seems much simpler to just use an effect here.

Closes #12936

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 1534c1d

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

[Rich-Harris]

One thing that occurs to me: we're not untracking the callback (either on main or in this PR). Should we be?

[paoloricciuti]

One thing that occurs to me: we're not untracking the callback (either on main or in this PR). Should we be?

It was untracked in the original implementation.

However i think @trueadm was building all this to avoid setting state in an effect (also to future proof it when branching is instroduced. Couldn't this be simply implemented in user land?

[paoloricciuti]

This will only invoke the callback the first time around rather then whenever the linked value changes right?

[Rich-Harris]

No? It will run whenever get_value() changes

[paoloricciuti]

Duh, mobile phone wrap around made me dirty...sprry

[Rich-Harris]

You absolutely can do this in userland, yes: #12545 (comment)

In that comment I mention two drawbacks:

1. you have to initialise the declaration for it to work in SSR, and the effect will run the first time regardless (i.e. if you're doing something like $state.snapshot(blah), then blah will immediately get snapshotted twice)
2. you have to be careful about untracking

But both of these cut both ways. Maybe I want the effect to run the first time! And maybe I want to have control over whether it should re-run when non-untracked dependencies change.

And the SSR issue could be solved with a simple userland helper of the kind that people have already built:

let a = $state(0);
let b = $state();

watch(() => a, (a, initial) => {
  b = a;
});

And here's something you can't do with $state.link — you can only deal with top-level variables, not properties:

$effect(() => {
  some.deeply.nested.property = whatever;
});

avoid setting state in an effect

We do want to discourage setting state in effects, for sure. But there are cases — the very cases where you'd use $state.link — where it's useful. And you certainly don't want to set state in deriveds.

So... yeah. Honestly I'm back to wondering 'why the hell do we need this?'. The only reason I can think of right now is so that we can make non-bindable props readonly without sacrificing ergonomics too badly.

[trueadm]

I don't think this should be an effect at all. We want it to be read when accessing the linked state and the link has been updated. An effect will mean it runs whenever the link updates but this can lead to sync bugs. Which I can explain better tomorrow, but it's late here.

[Rich-Harris]

Whether it uses the effect mechanism internally or not, this is, inescapably, an effect. It's a callback that by design does something when state changes, which is the very definition of the word. It's not a derived in any legitimate sense of the word, and as I say we definitely don't want deriveds to have side-effects

[trueadm]

@Rich-Harris It's a grey area, but ultimately it's a reaction/computation. It's not an effect and it's not a derived.

let count = $state(0);

let double = $state.link(count * 2, (doubled)=>{
  double = doubled;
});

count++;
console.log(double); // 2

[Rich-Harris]

If that's an important characteristic then it definitely needs to be tested.

A compromise option could be to remove the callback — if you need to do something when the state changes, use effects. I know I originally advocated for the callback, but now that we've shipped it I hate it. I had a niggling doubt when documenting the callback earlier, and I should have paid attention to it —there's no way to know, inside the callback, whether the linked state is newer than the original or vice versa, which feels like a pretty common requirement. In the docs I settled for a 'draw the rest of the owl' hasUnsavedChanges boolean, but that's a cop-out — the reality is that as soon as you're dealing with temporal concepts you need effects.

It still feels like it's probably unnecessary surface area even without the callback, but definitely to a lesser extent.

[trueadm]

See #12941 for the more complete fix. However, the big issue here is mutations as now the 2nd argument doesn't update when something inside it changes. So I feel like the second argument is problematic after all.

[Rich-Harris]

This PR removes the second argument: #12942

[Rich-Harris]

Closing, as $state.link was removed in #12942