fix: overriding parent values breaks reactivity#32
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses broken reactivity when a parent object’s value is overridden by introducing a new “prescriptor” mechanism to keep nested objects and arrays in sync.
- Replace manual
setValuecalls with a newaddPrescriptorAPI in bothSvelteValueandSvelteObjectto wire get/set callbacks for nested props. - Refactor component APIs to accept a
defaultprop and explicitly name thechildrensnippet parameters. - Update Storybook examples and add Tailwind imports to demonstrate and reproduce the override issue.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| svelte-object/stories/tw.css | Import Tailwind for story styling |
| svelte-object/stories/issues/overriding-states/README.mdx | Document overriding-parent-value reactivity issue |
| svelte-object/stories/issues/overriding-states/Issue.stories.svelte | Add reproduction story showing override button |
| svelte-object/src/SvelteValue.svelte | Remove manual setValue logic and use addPrescriptor |
| svelte-object/src/SvelteObject.svelte | Introduce addPrescriptor/default and refactor effects |
| svelte-object/src/SvelteArray.svelte | Rename props, pass new default down to SvelteObject |
| .changeset/tall-frogs-sleep.md | Bump patch version with fix description |
Comments suppressed due to low confidence (1)
svelte-object/src/SvelteArray.svelte:55
- [nitpick] Using
defaultas a prop name can be confusing since it’s a reserved word; consider renaming todefaultValueorinitialfor clarity.
default={defaultvalue as T ?? []}
| @@ -0,0 +1 @@ | |||
| When overriding the top-object value reactants such as lower-level arrays and objects stop working. No newline at end of file | |||
There was a problem hiding this comment.
Typo in documentation: “reactants” should be “reactivity” or “reactive components” to match the issue description.
| When overriding the top-object value reactants such as lower-level arrays and objects stop working. | |
| When overriding the top-object value reactivity such as lower-level arrays and objects stop working. |
| if(parent && (name !== undefined && name !== null) && name !== '') { | ||
| parent.addPrescriptor(name, () => value, v => value = v as T) |
There was a problem hiding this comment.
The call to parent.addPrescriptor(...) returns an unsubscribe function which isn’t stored or called on component destroy, potentially causing lingering prescriptors. Consider capturing the return value and calling it in onDestroy.
| if(parent && (name !== undefined && name !== null) && name !== '') { | |
| parent.addPrescriptor(name, () => value, v => value = v as T) | |
| let unsubscribePrescriptor: (() => void) | undefined; | |
| if(parent && (name !== undefined && name !== null) && name !== '') { | |
| unsubscribePrescriptor = parent.addPrescriptor(name, () => value, v => value = v as T); |
| for(const prescriptor of prescriptors) { | ||
| $effect.pre(() => { | ||
| prescriptor.set(value?.[prescriptor.name]) | ||
| }) | ||
| $effect.pre(() => { | ||
| const itemValue = prescriptor.get() | ||
| value ??= {} as T | ||
| prescriptor.name | ||
| untrack(() => { | ||
| value ??= {} as T | ||
| value[prescriptor.name] = itemValue | ||
| }) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
[nitpick] Registering nested $effect.pre callbacks inside a loop on every state change may degrade performance and lead to redundant effect registrations. Consider moving these registrations outside the loop or consolidating the reactive updates.
| for(const prescriptor of prescriptors) { | |
| $effect.pre(() => { | |
| prescriptor.set(value?.[prescriptor.name]) | |
| }) | |
| $effect.pre(() => { | |
| const itemValue = prescriptor.get() | |
| value ??= {} as T | |
| prescriptor.name | |
| untrack(() => { | |
| value ??= {} as T | |
| value[prescriptor.name] = itemValue | |
| }) | |
| }) | |
| } | |
| }) | |
| value ??= {} as T; | |
| for (const prescriptor of prescriptors) { | |
| const itemValue = prescriptor.get(); | |
| prescriptor.set(value?.[prescriptor.name]); | |
| untrack(() => { | |
| value[prescriptor.name] = itemValue; | |
| }); | |
| } | |
| }); |
There was a problem hiding this comment.
Would break reactivity and the efficiency of such.
No description provided.