-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: add onchange
option to $state
#15069
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 35e2afe 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 |
preview: https://svelte-dev-git-preview-svelte-15069-svelte.vercel.app/ this is an automated message |
|
Would there also be an |
Yes (uh I need to update types for that too) |
I like this, but also I'm thinking that there are more cases where you get a |
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. |
This would have helped me several times! Figuring out how to watch deep updates was very unintuitive to me. |
Recapping discussion elsewhere — I think we made a mistake with the implementation of let obj = {};
let a = $state(obj);
let b = $state(obj);
console.log(a === b); // false, which is good
let c = $state(b);
console.log(b === c); // true, which is bad There's no real point in having svelte/packages/svelte/src/internal/client/proxy.js Lines 32 to 35 in de94159
It would have been better if existing proxies passed directly to 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 to let obj = { count: 0 };
let a = $state(obj, {
onchange() {
console.log('a changed');
}
});
let b = $state(obj, {
onchange() {
console.log('b changed');
}
});
let c = $state(b, {
onchange() {
console.log('c changed');
}
}); I think one reasonably sensible solution would be to throw an error in the |
I think it'd probably be best if |
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: let items = $state([...]);
let selected = $state(items[0]);
function reset_selected() {
selected.count = 0;
} That doesn't work if we snapshot at every declaration site (and it certainly 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 let c = $state(b, {
onchange() {...}
}); let c = $state({}, {
onchange() {...}
});
c = b; |
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 let items = $state([...]);
let selected = $state.link(items[0]);
function reset_selected() {
selected.count = 0;
} |
Not sure how I feel about this. Shouldn't this just be some kind of effect, that watches deeply?
|
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 around Opened #15073 to address an issue with array mutations causing |
* only call onchange callbacks once per array mutation * fix * fix
There's no version of that that's more readable than including it in the declaration. Using single character names for everything where possible, to highlight the difference, this is 36 characters... let x = $state(y, { onchange: z }); ...while this is 41: let x = $state(y);
$state.changed(x, z); So we're off to a bad start. But then consider the fact that you're basically inviting people to separate the declaration from the change handler: let x = $state(y);
// lots of code...
$state.changed(x, z); Can you tell, quickly, whether a given piece of state has a change handler? If not, then it's now less readable in two ways. But there's more! We've subtly changed the meaning of the state binding. Everywhere else you see What if the first argument isn't a local state binding? I'd expect this to fail... let potato = 'yukon gold';
$state.changed(potato, () => ...); ...because let { potato } = $props();
$state.changed(potato, () => ...); Aren't props state? What about deriveds? (The answer is no, this feature is for sources, not props or derived state; it doesn't make sense in those contexts.) Suddenly we have to devote a bunch of space in the documentation to explaining that nuance, all while making the API itself — you guessed it — less readable. And doesn't it kind of look like an effect? $state.changed(foo, () => {
blah(foo);
});
$effect(() => {
blah(foo);
}); So now we have these two things that, to the untrained eye, look quite similar (even though they're actually very different things). Pretty confusing. Remember what this feature is for: it's so that you, the declarer of a piece of state, have a simple way to say 'run this code when this state changes'. It would be weird not to do that at the declaration site. Shorter answer: no. |
I have some questions to get a little bit of clarity on the intended use cases for the onchange callback. 1.) Is this intended to perform side effects whenever the state changes? Or put differently, is this analolgus to the $effect rune but limited in scope to changes to a specific declared variable?
2.) Is this intended to apply transformations to the state of a variable whenever it changes?
// This would trigger only one view update instead of multiple if these transformations were performed within an $effect rune.
// Using a callback within the $state willChange handler
let text = $state('', (newValue) => {
let transformedString = newValue.slice(0,10);
transformedString.toLowerCase();
return transformedString;
});
// This change would trigger only one DOM update.
// Without a
text = "HeLLo, WOrld";
// Using an $effect rune to transform the string would lead to multiple view updates for one state change.
let text = $state('');
$effect(() => {
let transformedString = text.slice(0,10);
transformedString.toLowerCase();
text = transformedString;
});
text = "HeLLo, WOrld"; 3.) When does the onchange callback run? Does it happen before the DOM is updated? After? |
Yeah we could but tbf I don't really see the point |
Is a sort of in-between: it doesn't carry the same weight and even flexibility of an effect (it's not fine grained, it doesn't auto track variables etc etc). But it has the advantage of deeply notify without having to read the values. And you can also use it without an
I wouldn't say it's a use case...it can be done but I would do that at the assignment site...if you reassign the same state in this callback you will cause an unnecessary trigger of everything is listen to it. If you really want to make sure everything is setting this variable has a transformation applied use a function or a getter to pass it around.
Immediately after the value is set so before the Dom update |
I haven't fully thought this through but I think it might be cool to also add Benefits:
|
Thanks for the clarification!
If the onchange callback is called immediately after the value is set, could a copy the old value be passed as a single parameter? For example: let foo = $state(1, (oldValue) => {
if (foo > oldValue) {
// Do something
} else {
// Do something else
}
}); |
As I've said before it's very simple to get the previous value if you really need it so I don't think we should add it. And that would also require screenshot every single value which is very bad for performance |
It doesn't really make sense to have |
Which behavior is expected for subobjects that don't belong to the state with
Should there be a guard against infinite self-triggering like with $effect? |
I don't think we can do anything because the
I think we can do something about this...let me see
We can...but maybe it's an overkill? With effects it might be easy to don't realise that you are reading and writing but with this it should be easier to spot. |
How about this case: let first = $state({ deep: "object" }, {
onchange() {
console.log("first changed")
}
})
let second = $state({ another: { deep: 123 } }, {
onchange() {
console.log("second changed")
}
})
// all good, right? how about now
second = first
// second === first is true now
first.deep = "new value"
// which onchange triggers here? IMO I think it's okay if both onchanges trigger |
packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js
Outdated
Show resolved
Hide resolved
packages/svelte/tests/compiler-errors/samples/runes-wrong-state-args/_config.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Matei Trandafir <[email protected]>
Deriveds are pull based only, we only push the notion they are possibly dirty. Deriveds only become dirty when their dependencies invalidate them, which can differ depending on various heuristics (if the derived is owned vs unowned etc). So there's no guarantee on when we can safely invoke |
I've added a new commit to make so that if you pass a proxy to proxy it will "stack" the |
Are we looking for the This would prevent code from being written where the options aren't statically analyzable at compile time (which might always be considered a feature), but it would give us more control in the future with additional state options, so they don't all need to be included as-is in the client code. |
I actually wanted to add an error in case it was not specified inline but for different reasons: in my latest commit I'm changing the original object to chain the onchange. That might result in weird stuff if the object was shared. So I think I will actually add an error if the options are not specified inline which will also satisfy your point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised this does not work with nested $state
fields in classes. I don't think it should, as these are new states with possibly new onchange
fields, but still this should be noted
@@ -147,6 +147,35 @@ person = { | |||
|
|||
This can improve performance with large arrays and objects that you weren't planning to mutate anyway, since it avoids the cost of making them reactive. Note that raw state can _contain_ reactive state (for example, a raw array of reactive objects). | |||
|
|||
## State options | |||
|
|||
Both `$state` and `$state.raw` can optionally accept a second argument. This allows you to specify an `onchange` function that will be called synchronously whenever the state value changes (for `$state` it will also be called for deep mutations). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both `$state` and `$state.raw` can optionally accept a second argument. This allows you to specify an `onchange` function that will be called synchronously whenever the state value changes (for `$state` it will also be called for deep mutations). | |
Both `$state` and `$state.raw` can optionally accept a second argument. This allows you to specify an `onchange` function that will be called synchronously whenever the state value changes. For a deep `$state` proxy (of a POJO/array), `onchange` is also called for deep mutations, but not for `$state` fields in classes, as these are owned by the corresponding class instance. This includes classes from `svelte/reactivity`, such as [SvelteMap](svelte-reactivity#SvelteMap). |
I think I fixed almost everything...there's one bug where if you remove an element from the array and push back to it again it invoke the onchange twice the first time but the rest should work. However wants to try and break this is well accepted |
Closes #15032
This adds an
onchange
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 function
assignable_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 with
set
later...much nicer.Edit edit edit: I've also implemented how to move the proxying logic inside
set
...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 thisis compiled toif 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 internals
get_options
so that when it's reassigned the proxy reassignment looks like thisthe same is true for classes...there however i've used an extra private identifier to store the optionsis compiled tothere'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
and updating bar will only trigger the update for
foo
.Finally the
onchange
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
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint