Skip to content
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

Svelte 5: Reactivity bug with spread operator #10065

Closed
abdel-17 opened this issue Jan 3, 2024 · 7 comments · Fixed by #10071
Closed

Svelte 5: Reactivity bug with spread operator #10065

abdel-17 opened this issue Jan 3, 2024 · 7 comments · Fixed by #10071

Comments

@abdel-17
Copy link

abdel-17 commented Jan 3, 2024

Describe the bug

Spreading an object with reactive props doesn't work as expected.

<script>
	let pressed = $state(false);

	const props = {
		get "aria-pressed"() {
			return pressed;
		},
		onclick: () => {
			pressed = !pressed;
		}
	}
</script>

<!-- "aria-pressed" is not reactive -->
<button {...props}>
	{pressed ? "On" : "Off"}
</button>

Reproduction

REPL

Logs

No response

System Info

Macbook Pro 2020 Intel i5, macOS 14.2

Severity

annoyance

@brunnerh
Copy link
Member

brunnerh commented Jan 3, 2024

Looks like the compiler does not generate a $.render_effect block for the spread if the spread object is not signal-based. This causes the signal emitted from the getter to not trigger an update.

Spreading this on the other hand would work:

const props = $derived({
	"aria-pressed": pressed,
	onclick() { pressed = !pressed },
});

@abdel-17
Copy link
Author

abdel-17 commented Jan 3, 2024

Is this fine-grained or does the entire object get revalidated?

@brunnerh
Copy link
Member

brunnerh commented Jan 3, 2024

This would recreate the object on change.

What is the use case for doing this, though?
Instead of creating an object and spreading it, the properties just could be set directly in markup.

@abdel-17
Copy link
Author

abdel-17 commented Jan 3, 2024

I was migrating some melt-ui code to runes. Props are spread into elements react-aria style.

<script>
  const { elements: { root } } = createLabel();
</script>

<!-- In the migrated code, `root` is an object rather than a store -->
<label use:root {...$root} />

@HighFunctioningSociopathSH
Copy link

This would recreate the object on change.

What is the use case for doing this, though? Instead of creating an object and spreading it, the properties just could be set directly in markup.

Could you please take a look at this issue (#10007) revolving around the same problem? I made the issue about a week ago but didn't get any answers and I think it's kind of important as it can cause serious performance issues and unexpected behaviors.

@abdel-17
Copy link
Author

abdel-17 commented Jan 3, 2024

This would recreate the object on change.

What is the use case for doing this, though? Instead of creating an object and spreading it, the properties just could be set directly in markup.

Could you please take a look at this issue (#10007) revolving around the same problem? I made the issue about a week ago but didn't get any answers and I think it's kind of important as it can cause serious performance issues and unexpected behaviors.

I think this is expected, though. $derived is just a cached function call. If you want the individual properties to be reactive, you need to use getters.

const x = $derived(x_expression);
const y = $derived(y_expression);
return {
  get x() { return x },
  get y() { return y }
}

@brunnerh
Copy link
Member

brunnerh commented Jan 3, 2024

Let's not discuss separate issues here.

As for the lack of responses: Most maintainers are probably still on holiday, and I am not a maintainer, I just meddle 😅. Also, Svelte 5 is not even officially released yet and there are quite a few issues that still need to be ironed out - there is no need to be impatient.

dummdidumm added a commit that referenced this issue Jan 3, 2024
The objects could contain getters with reactive values, so we play it safe and assume they're always reactive
fixes #10065
dummdidumm added a commit that referenced this issue Jan 3, 2024
- the objects could contain getters with reactive values, so we play it safe and assume they're always reactive - fixes #10065
- isolate spreads with call expression similar to how we do it with other effects -fixes #10013
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 a pull request may close this issue.

3 participants