Skip to content

feat: adds $state.link rune #12545

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 31 commits into from
Aug 20, 2024
Merged

feat: adds $state.link rune #12545

merged 31 commits into from
Aug 20, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jul 22, 2024

This PR adds the $state.link rune, which acts like $state in that you can write to the value and if it's an object or array, it will be deeply reactive. However, the difference is that unlike $state where the passed in value is the initial value, $state.link will keep up to day with consistent one-way link from the passed in value. Mutating the linked state locally will only mutate the value locally – it will not propagate the state change to the value that it was linked to.

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

  // `child_value` is linked to `parent_value`
  let child_value = $state.link(parent_value);

  parent_value = 10;

  console.log(child_value); // 10

  child_value = 20;

  console.log(child_value); // 20

  // `parent_value` remains the same
  console.log(parent_value); // 10
</script>

This PR still needs docs, so marking as WIP for now.

closes #12364

Copy link

changeset-bot bot commented Jul 22, 2024

🦋 Changeset detected

Latest commit: 3cd37ac

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

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Are we sure we want to proxify the incoming value? If you pass in a raw value it would get proxified even if you don't want that.
On the other hand, if someone would want to mutate the linked value, but not mutate the original value, they would use something like $state.snapshot, but if the result isn't proxified you won't have a way of mutating it - mhm..

Almost feels like $state.link should get passed an arrow function so that you can control that:

// deep copy, mutations are reactive
const link1 = $state.link(() => { const copy = $state($state.snapshot(original)); return copy; });
// deep copy, mutations are not reactive
const link2 = $state.link(() => $state.snapshot(original));
// shallow copy, mutations maybe reactive but go through to original
const link3 = $state.link(() => original);

Alternatively there's a second argument that controls that, or there's $state.link and $state.raw_link

@trueadm
Copy link
Contributor Author

trueadm commented Jul 23, 2024

Are we sure we want to proxify the incoming value? If you pass in a raw value it would get proxified even if you don't want that. On the other hand, if someone would want to mutate the linked value, but not mutate the original value, they would use something like $state.snapshot, but if the result isn't proxified you won't have a way of mutating it - mhm..

Almost feels like $state.link should get passed an arrow function so that you can control that:

// deep copy, mutations are reactive
const link1 = $state.link(() => { const copy = $state($state.snapshot(original)); return copy; });
// deep copy, mutations are not reactive
const link2 = $state.link(() => $state.snapshot(original));
// shallow copy, mutations maybe reactive but go through to original
const link3 = $state.link(() => original);

Alternatively there's a second argument that controls that, or there's $state.link and $state.raw_link

Yep, this should apply deep-state. The intention is to add a $state.raw.link for raw state later on once we have that rune, I avoided talking about that here as that $state.raw hasn't shipped yet.

@Rich-Harris
Copy link
Member

(Given how carefully the React team designs APIs, this difference is probably worth interrogating in more depth, but the point is that in its current state this is very much not a clone of useOptimistic.)

Didn't want to leave this unaddressed. I took a closer look at useOptimistic and found the behaviour quite surprising:

Screen.Recording.2024-08-20.at.12.18.14.PM.mov

Notice that, since optimistic updates are 'replayed' on top of the new state, messages can appear to be both sent and sending. I'm left wondering if I've just misunderstood something, because that doesn't seem like a particularly useful API.

Either way, it allays my concern that we missed something. $state.link wouldn't be a good API for optimistic updates because it wouldn't do a good job of handling multiple updates interleaved with server responses — personally my inclination would be to be a bit less clever and a bit more smart:

Screen.Recording.2024-08-20.at.12.41.52.PM.mov

@7nik
Copy link
Contributor

7nik commented Aug 20, 2024

Didn't want to leave this unaddressed. I took a closer look at useOptimistic and found the behaviour quite surprising:

I found a few issues with this hook not working well with multiple concurrent operations, as shown in your video.
Another magical thing, not even mentioned in the docs, is that the addOptimistic must be called inside an async function— it gets "bound" to the function and removes the optimistic value when the function finishes. And the updateFn can be called really a lot of times because optimistic values are added one by one at each change of the parent state — a typical React move.

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Yay! Hehe. Thanks, fine gents. Don't you DARE take it away from me now!! 😄

Rich-Harris added a commit that referenced this pull request Aug 20, 2024
@Rich-Harris
Copy link
Member

Don't hate me, but I am thinking about it. The implementation in this PR didn't quite work, and the fixed version is simple enough that I can't really think of a good reason this shouldn't just exist in userland:

export function source_link(get_value, callback) {
var s = source(/** @type {V} */ (undefined));
var ran = false;
callback ??= (value) => set(s, value);
render_effect(() => {
if (ran) {
callback(get_value());
} else {
s.v = get_value();
}
});
ran = true;
return function (/** @type {any} */ value) {
return arguments.length === 1 ? set(s, /** @type {V} */ (value)) : get(s);
};
}

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Damn! LOL.

I won't say a thing. I already exposed my issue and how un-sveltey the workaround felt. In the end, you're the gatekeeper between us mortals and the almighty Svelte. I just want to love Svelte v5 as much and even more than v4. Version 4 made me enjoy UI development for the first time ever, I think. I've been back-end for 20 years.

Rich-Harris added a commit that referenced this pull request Aug 21, 2024
Rich-Harris added a commit that referenced this pull request Aug 21, 2024
* Revert "breaking: remove `$state.link` callback (#12942)"

This reverts commit 0b51ff0.

* Revert "feat: adds $state.link rune (#12545)"

This reverts commit 63ec2e2.

* put changesets back

* changeset

* merge main
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.

Provide more control over what gets proxified, pontentially making it opt-in
8 participants