feat: use state proxy ancestry for ownership validation #11184
Conversation
🦋 Changeset detectedLatest commit: 05856f3 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 |
| @@ -2,8 +2,7 @@ | |||
| import Counter from './Counter.svelte'; | |||
| import { global } from './state.svelte.js'; | |||
|
|
|||
| let object = $state({ count: 0 }); | |||
| global.object = object; | |||
| global.object = { count: 0 }; | |||
There was a problem hiding this comment.
The idea of this test is to check that ownership is widened when some at-first-local state is passed to some global state and then is also treated as globally available and therefore allowed to mutate.
Think of a form where the user does some inputs and then saves this to some settings that are then globally shared.
Is this still supported with the new approach?
There was a problem hiding this comment.
addressed this above:
...the parent of
objectis stillnull, notsome_other_state. This means one of the tests needed adjusting, but I think that's okay.
We could make it work with a bit of finagling but I'm not convinced we should. I don't think you'd ever need to write code like that in real life, I think the mental model is clearer in this PR
There was a problem hiding this comment.
I don't know, feels like it's only a matter of time until some combination of reassignments will lead to such a situation. Could we do something like "when state is assigned to a different state, lift ownership of assigned state completely"? This way don't need to mess with having multiple parents etc etc, we're just lifting ownership at the time of reassignment. You're saying this situation is super rare, so this would trade a rare false positive (bad) with a rare false negative (okay).
There was a problem hiding this comment.
But what if you have a case like this instead?
-let object = $state({ count: 0 });
-global.object = object;
+let object = $state({ nested: { count: 0 } });
+global.nested = object.nested;Does the parent of object.nested change from object to global? Or at that point do we need to allow things to have multiple parents, with all the extra complexity (and memory leak risk) that implies? Just seems like the dictionary definition of YAGNI to me.
There was a problem hiding this comment.
The parent doesn't change at all, it stays the same. But whatever defines the allowed set of owners switches from "this set of components" to "everything" for object.nested.
There was a problem hiding this comment.
I see. It would definitely add some complexity (we would need a new special value for indicating universal ownership rather than using null to indicate 'not owned by anyone', and we'd need a get_all_owners function), so my inclination is still to file it under YAGNI. If it happens in the wild, we could revisit but it doesn't feel like something we should be defensive about
There was a problem hiding this comment.
I pushed a commit implementing what I had in mind - it's only a handful lines of code
| } else if (object && typeof object === 'object') { | ||
| if (object[ADD_OWNER]) { | ||
| // this is a class with state fields | ||
| render_effect(() => { |
There was a problem hiding this comment.
why is a render effect necessary here?
|
|
||
| if (metadata) { | ||
| // this is a state proxy, add owner directly | ||
| (metadata.owners ??= new Set()).add(owner); |
There was a problem hiding this comment.
Pretty sure this is wrong, because null means "everything's allowed", causing unwanted narrowing in a case like this. Looking into it.
| object[ADD_OWNER](owner); | ||
| }); | ||
| } else { | ||
| // recurse until we find a state proxy |
There was a problem hiding this comment.
should we bail on non-POJOs here? IIUC this would also recurse into DOM elements, for example
There was a problem hiding this comment.
ah yeah, probably a good idea
| ? // @ts-expect-error | ||
| new Set([current_component_context.function]) | ||
| : null | ||
| : owners && new Set(owners); | ||
| : parent.owners; |
There was a problem hiding this comment.
This (084aabf) defeats the object. We can't just share an owners set between different objects, because then if you have something like this...
<script>
import Counter from './Counter.svelte';
let object = $state({
shared: { count: 0 },
notshared: { count: 0 }
});
</script>
<Counter
bind:shared={object.shared}
notshared={object.notshared}
/>...adding <Counter> as an owner of shared will also add it as an owner of notshared.
There was a problem hiding this comment.
Is that really necessary to differentiate? Feels like a pretty rare case, and it would only be a false negative, not a false positive.
If you want to keep the behavior, then we need a separate special "ownerless" value, regardless of my change to widen ownership because of the bug that otherwise creeps in I mentioned here #11184 (comment)
Edit: No your change should be fine, because of the "traverse up the parent chain" behavior which would eventually find the ownerless parent and conclude it's fine to mutate.
Follow-up to #11136 (and #11094 and #11117).
This changes the ownership validation mechanism to be lazier. Instead of
add_ownerrecursively adding an owner to everything, it stops when it finds a state proxy. Properties of those state proxies, if they are also state proxies, are linked to the parent.When it comes time to check if a given mutation is allowed, rather than just looking at the owners of the proxy being modified we also walk up the tree. This should significantly reduce cost — not only do we avoid setting ownership in many places, but we also avoid eagerly creating state proxies for things that would otherwise not need them.
The mental model is a little bit simpler: the 'root' is ultimately the
$statedeclaration, and parent relationships are fixed. In other words here......the parent of
object.nestedisobject, and the parent ofobjectisnull. If we then do this......the parent of
objectis stillnull, notsome_other_state. This means one of the tests needed adjusting, but I think that's okay.Submitting this as a draft PR so that we can see if there are any cases that would lead to false positive warnings (bad), false negatives (okay, depending on the situation) or memory leaks (potentially very bad, unless it's an absurdly contrived scenario).
TODO
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint