Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: addonchange option to$state#15069

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
paoloricciuti wants to merge66 commits intomain
base:main
Choose a base branch
Loading
fromstate-onchange
Open

Conversation

paoloricciuti
Copy link
Member

@paoloricciutipaoloricciuti commentedJan 20, 2025
edited
Loading

Closes#15032

This adds anonchange option to the$state runes as a second argument. This callback will be invoked whenever that piece of state changes (deeply) and could help when building utilities where you control the state but want to react deeply without writing an effect or a recursive proxy.

Edit:@brunnerh proposed a different transformation which is imho more elegant...gonna implement that tomorrow...instead of creating a state and a proxy externally we can create a new functionassignable_proxy that we can use in the cases where we would do$.state($.proxy()) do that we can not declare the extraconst orprivate_id. We could also pass a parameter toset to specify if we should proxify or not and that could solve the issue ofget_options.

Edit edit: I've implemented the first of the above suggestions...gonna see what is doable withset later...much nicer.

Edit edit edit: I've also implemented how to move the proxying logic insideset...there's one issue which i'm not sure if we care about: if you reassign state in the constructor of a class we don't call set do it's not invoking the onchange...we can easily fix this with another utility function that does both things but i wonder if we should.

A couple of notes on the implementation. When initialising a proxy that is also reassigned we need to pass the options twice, once to the source and once to the proxy...i did not found a more elegant way so for the moment this

letproxy=$state({count:0},{onchange(){}});

is compiled to

letstate_options={onchange(){}},proxy=$.state($.proxy({count:0},state_options),state_options);

if the proxy is reassigned.

Then when the proxy is reassigned i need to pass the same options back to the newly created proxy. To do so i exported a new function from the internalsget_options so that when it's reassigned the proxy reassignment looks like this

$.set(proxy,$.proxy({count:$.get(proxy).count+1},$.get_options(proxy)))

the same is true for classes...there however i've used an extra private identifier to store the options

classTest{proxy=$state([],{onchange(){}})}

is compiled to

classTest{#state_options={onchange(){}};#proxy=$.state($.proxy([],this.#state_options),this.#state_options);getproxy(){return$.get(this.#proxy);}setproxy(value){$.set(this.#proxy,$.proxy(value,$.get_options(this.#proxy)));}}

there's still one thing missing: figure out how to get a single update for updates to arrays (currently if you push to an array you would get two updates, one for the length and one for the element itself.

Also also currently doing something like this

letfoo=$state({},{onchange:()=>console.log('foo')});letbar=$state(foo,{onchange:()=>console.log('bar')});

and updating bar will only trigger the update forfoo.

Finally theonchange function is untracked since it will be invoked synchronously (this will prevent updating from an effect adding an involountary dependency.

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 withfeat:,fix:,chore:, ordocs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code withinpackages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests withpnpm test and lint the project withpnpm lint

max-got, Ocean-OS, gyurielf, unlocomqx, jjones315, aecea, Froidland, jakob-kruse, Clembs, rChaoz, and 13 more reacted with thumbs up emojirChaoz, ivenuss, llama-for3ver, difsch2077, urboifox, JLAcostaEC, and aster-void reacted with rocket emojispizzy-dev, lucaesposto, TheBeachMaster, ieedan, 0gust1, difsch2077, urboifox, and aster-void reacted with eyes emoji
@changeset-botchangeset-bot
Copy link

changeset-botbot commentedJan 20, 2025
edited
Loading

🦋 Changeset detected

Latest commit:293fefb

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

This PR includes changesets to release 1 package
NameType
svelteMinor

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
Copy link
Member

preview:https://svelte-dev-git-preview-svelte-15069-svelte.vercel.app/

this is an automated message

@github-actionsGitHub Actions
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15069

@Ocean-OS
Copy link
Contributor

Would there also be anonchange option for$state.raw?

@paoloricciuti
Copy link
MemberAuthor

Would there also be anonchange option for$state.raw?

Yes (uh I need to update types for that too)

Ocean-OS and rChaoz reacted with thumbs up emoji

@Ocean-OS
Copy link
Contributor

Ocean-OS commentedJan 20, 2025
edited
Loading

I like this, but also I'm thinking that there are more cases where you get a$state from an external source (props, imported, etc), and you might want to synchronously observe their changes as well.
I'm not sure what the implementation of that would look like, however. Maybe something like a$watch rune, which was proposed somewhere.

@paoloricciuti
Copy link
MemberAuthor

I like this, but also I'm thinking that there are more cases where you get a$state from an external source (props, imported, etc), and you might want to synchronously observe their changes as well. I'm not sure what the implementation of that would look like, however. Maybe something like a$watch rune, which was proposed somewhere.

I think effect or derived with state is the good formula for that already...this aims to solve the problem of getting deep updates for a stateful variable that you own.

jjones315, Ocean-OS, rChaoz, and joelmukuthu reacted with thumbs up emoji

@jjones315
Copy link
Contributor

This would have helped me several times! Figuring out how to watch deep updates was very unintuitive to me.

@Rich-Harris
Copy link
Member

Recapping discussion elsewhere — I think we made a mistake with the implementation of$state(other_state):

letobj={};leta=$state(obj);letb=$state(obj);console.log(a===b);// false, which is goodletc=$state(b);console.log(b===c);// true, which is bad

There's no real point in havingc be a separate binding with the same value; if you want that just dolet c = b. But for some reason we did this:

// if non-proxyable, or is already a proxy, return `value`
if(typeofvalue!=='object'||value===null||STATE_SYMBOLinvalue){
returnvalue;
}

It would have been better if existing proxies passed directly to$state were snapshotted.


Most of the time this doesn't really matter, because people generally don't use state like that. But we do need to have some answer to the question 'what happens here?' more satisfying than 'we just ignore the options passed toc altogether:

letobj={count:0};leta=$state(obj,{onchange(){console.log('a changed');}});letb=$state(obj,{onchange(){console.log('b changed');}});letc=$state(b,{onchange(){console.log('c changed');}});

I think one reasonably sensible solution would be to throw an error in thelet c = $state(b, ...) case if options were passed to eitherb orc. In Svelte 6 we could go one of two ways — automatically snapshot, or error whenany state proxy is passed to$state.

Ocean-OS, SikandarJODD, gka, and rChaoz reacted with thumbs up emoji

@Ocean-OS
Copy link
Contributor

Ocean-OS commentedJan 21, 2025
edited
Loading

I think one reasonably sensible solution would be to throw an error in thelet c = $state(b, ...) case if options were passed to eitherb orc. In Svelte 6 we could go one of two ways — automatically snapshot, or error whenany state proxy is passed to$state.

I think it'd probably be best if$state proxies were snapshotted when passed to$state. You don't always know if you're getting a$state proxy (such as in props). I also think it'd be best ifthis sort of thing were to be fixed earlier, as this doesn't seem like somethinganyone would want/try to do.

@Rich-Harris
Copy link
Member

Actually, I take it back — in thinking about why we might have made it work that way in the first place, this case occurs to me:

letitems=$state([...]);letselected=$state(items[0]);functionreset_selected(){selected.count=0;}

That doesn't work if we snapshot at every declaration site (and itcertainly wouldn't make sense to snapshot on declaration but not on reassignment).

So I guess automatic snapshotting and erroring are both off the table — we need to keep the existing behaviour for$state(value) today and in Svelte 6. But I do think we should make it an error to do either of these in the case whereb is an existing state proxy (with or without an existing options object), because there's no way for Svelte to know whichonchange handler to fire when the state is mutated:

letc=$state(b,{onchange(){...}});
letc=$state({},{onchange(){...}});c=b;

@Ocean-OS
Copy link
Contributor

Ocean-OS commentedJan 21, 2025
edited
Loading

Actually, I take it back — in thinking about why we might have made it work that way in the first place, this case occurs to me:

letitems=$state([...]);letselected=$state(items[0]);functionreset_selected(){selected.count=0;}

That doesn't work if we snapshot at every declaration site (and itcertainly wouldn't make sense to snapshot on declaration but not on reassignment).

So I guess automatic snapshotting and erroring are both off the table — we need to keep the existing behaviour for$state(value) today and in Svelte 6.

I didn't think of that, actually. But couldn't that be fixed with$state.link? That way, we wouldn't have to worry about$state proxies referencing other$state proxies, unless that is the desired behavior:

letitems=$state([...]);letselected=$state.link(items[0]);functionreset_selected(){selected.count=0;}

@levibassey
Copy link

levibassey commentedJan 21, 2025
edited
Loading

Not sure how I feel about this. Shouldn't this just be some kind of effect, that watches deeply?

$effect(() => {    ...}, {deep: true})

@Rich-Harris
Copy link
Member

Shouldn't this just be some kind of effect

No. Avoiding effects is the whole point of this. Deep-reading is easy, but often when you want to respond to state changes, the state was created outside an effect (think e.g. an abstraction aroundlocalStorage).

Opened#15073 to address an issue with array mutations causingonchange to fire multiple times.

@paoloricciuti
Copy link
MemberAuthor

Any thought to doing something likelet foo = $state(watch({ bar :"value" }, () => handleChange()))

Instead of

let foo = $state({ bar :"value" }, () => handleChange())

In my opinion this is more composable and extendable. I know this is an older PR but to me the proposed API feels a bit off.

What's the value of thewatch function?

@rChaoz
Copy link
Contributor

rChaoz commentedMar 26, 2025
edited
Loading

Any thought to doing something like let foo = $state(watch({ bar :"value" }, () => handleChange()))

I think the issue here is, this makes it seem likewatch can be used without$state. Correct me if I'm wrong, but isn't theonchange functionality provided by$state wrapping the object deeply with a proxy? How couldwatch work without this?

@paoloricciuti
Copy link
MemberAuthor

Any thought to doing something like let foo = $state(watch({ bar :"value" }, () => handleChange()))

I think the issue here is, this makes it seem likewatch can be used without$state. Correct me if I'm wrong, but isn't theonchange functionality provided by$state wrapping the object deeply with a proxy? How couldwatch work without this?

Yeah watch without$state would just be a worse effect.

I mean we could make it work but I just don't understand what the advantage would be (it also looks very ugly)

@paoloricciuti
Copy link
MemberAuthor

Watch would likely wrap the value in a proxy, this is basically a copy of a function I wrote to solve this problem before this PR. It could also be done at compile time but I think you lose some of the extensibility. Watch is also a place holder name it would be “track” but to me if someone comes to this file with watch it’s fairly obvious what the callback it doing. If I see a second arg in the state rune it’s much less obvious what the callback is for.

What this PR does it's different from using effect since it's invoking the callback synchronously... it's also doing it deeply without the need to create a source for the whole object.

@paoloricciuti
Copy link
MemberAuthor

What's not clear about an object withonchange property? Doesn't theonchange signal that you want that callback to be invoked when the state value changes?

@paoloricciuti
Copy link
MemberAuthor

Nope, it's just the compiler that is unwrapping the object to pass just the onchange to the function so the API is still the same

@Rich-Harris
Copy link
Member

@jjones315 Please don't delete comments from threads, it makes it very difficult for people to understand what (for example) other people are replying to. This repo is a public record. We'll hide irrelevant comments if that's necessary to declutter the thread

@Rich-Harris
Copy link
Member

This is good but I'm very concerned about the cost of creating a new set and a new wrapper function on every new proxy. I think we have to come up with a way around that.

Here's what I'm thinking: a top-level source can have anonchange function that gets called insideset (notinternal_set), while a proxy always has anonchanges array (not set, for reasons explained in a sec) which is usually empty. When a proxy is created as a child of another proxy, it just gets that same array. The proxy itself is responsible for calling all theonchanges when a property is updated. Onproxy.property = other_proxy, theonchanges belonging toproxy are appended to theonchanges belonging toother_proxy; ondelete proxy,other_proxy has theproxyonchanges removed.

It's an array rather than a set because you could have duplicates:

functiononchange(){console.log('changed');}letproxy=$state({},{ onchange});letother_proxy=$state({},{ onchange});// `other_proxy` inherits `onchange` from `proxy`proxy.property=other_proxy;// `onchange` is removed from `other_proxy`deleteproxy.property;

If it was a set, the add/remove would be assymetrical, andother_proxy would lose itsownonchange.

There is a caveat to this approach:

letproxy=$state({},{onchange:foo});letother_proxy=$state({},{onchange:bar});letmiddle={ other_proxy};proxy.middle=middle;

Here, wewouldn't want to passfoo toother_proxy, because then ondelete proxy.middle we would need to walk the object until we were able to remove it again which is costly.

As a result,proxy.middle.other_proxy.blah would on triggerbar, notfoo. Personally I think this is an acceptable trade-off.

Will hack on this

Ocean-OS and paoloricciuti reacted with thumbs up emoji

@paoloricciuti
Copy link
MemberAuthor

Let me know if you want to pair on this or if I can do anything to make your work easier 🤟🏻

@Rich-Harris
Copy link
Member

Gah I made some progress on this (compare/state-onchange...state-onchange-roots?expand=1) but there's a (obvious-in-hindsight) fatal flaw:

letitems=$state([...],{ onchange});constlast=items.pop();// should not trigger `onchange` as it's no longer in `items`, but it doeslast.foo='blah';

If theonchanges array is shared between parent and child, we can't remove the callback when an item is detached.

Think I need to step away from this PR for a bit because there are other demands on my attention, but a sketch of apossible alternative approach:

  • we distinguish between root proxies and child proxies
  • root proxies can have anonchange callback
  • child proxies have aparent if they are a child of a root with anonchange or a child of a child with aparent
  • when a mutation happens (i.e. via theset ordeleteProperty traps) we walk up theparent chain to a root. If the chain is broken somewhere (because an object was detached from a state-with-onchange tree) the callback does not fire
  • existing state proxies cannot be added to a state-with-onchange tree — only POJOs, non-proxyable objects, and primitives. this ensures that a child proxy has either 0 or 1 parents (otherwise this doesn't work)
  • the exception to this is when mutating an array with e.g.array.splice(...) orarray.reverse(), since ifarr[i] is assigned toarr[j] it will already be a state proxy

In other words we introduce some new constraints to enable a memory-friendly implementation, but these constraints only apply to state that usesonchange.

@paoloricciuti
Copy link
MemberAuthor

I think this was basically my original implementation...I don't think a lot of people will pass proxies to proxies but I wonder if this limitation could be confusing.

What if we stick to an array (not a set) and we dedupe the functions when calling them?

@dummdidumm
Copy link
Member

existing state proxies cannot be added to a state-with-onchange tree — only POJOs, non-proxyable objects, and primitives. this ensures that a child proxy has either 0 or 1 parents

I think that would be a dealbreaker. Consider this situation.

<script>import {localStorage }from'./somewhere';let todo=$state({});functionsave() {// localStorage uses onchange, so this will throwlocalStorage.state.todos.push(user);  }</script>....

I think something like this is prone to happen one way or the other.

@paoloricciuti
Copy link
MemberAuthor

Yeah I think it would be very confusing or annoying...is there a way to measure the memory footprint?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@Rich-HarrisRich-HarrisRich-Harris left review comments

@trueadmtrueadmtrueadm left review comments

@dummdidummdummdidummdummdidumm left review comments

@rChaozrChaozrChaoz left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

$state mutation callback
15 participants
@paoloricciuti@Rich-Harris@Ocean-OS@jjones315@levibassey@opensas@cgaaf@7nik@dummdidumm@torrfura@Leonidaz@rChaoz@trueadm@Conduitry@brunnerh

[8]ページ先頭

©2009-2025 Movatter.jp