Skip to content

fix: overriding parent values breaks reactivity#32

Merged
Refzlund merged 3 commits intomasterfrom
fix-overriding-states
Jul 3, 2025
Merged

fix: overriding parent values breaks reactivity#32
Refzlund merged 3 commits intomasterfrom
fix-overriding-states

Conversation

@Refzlund
Copy link
Owner

@Refzlund Refzlund commented Jul 3, 2025

No description provided.

@Refzlund Refzlund requested a review from Copilot July 3, 2025 11:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 setValue calls with a new addPrescriptor API in both SvelteValue and SvelteObject to wire get/set callbacks for nested props.
  • Refactor component APIs to accept a default prop and explicitly name the children snippet 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 default as a prop name can be confusing since it’s a reserved word; consider renaming to defaultValue or initial for 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
Copy link

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

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

Typo in documentation: “reactants” should be “reactivity” or “reactive components” to match the issue description.

Suggested change
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.

Copilot uses AI. Check for mistakes.
Comment on lines +167 to +168
if(parent && (name !== undefined && name !== null) && name !== '') {
parent.addPrescriptor(name, () => value, v => value = v as T)
Copy link

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
Comment on lines +108 to +122
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
})
})
}
})
Copy link

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

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

[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.

Suggested change
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;
});
}
});

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

Choose a reason for hiding this comment

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

Would break reactivity and the efficiency of such.

@Refzlund Refzlund merged commit 0918ef0 into master Jul 3, 2025
1 check passed
@Refzlund Refzlund deleted the fix-overriding-states branch July 3, 2025 11:45
@github-actions github-actions bot mentioned this pull request Jul 3, 2025
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.

2 participants