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

[Svelte5] Object are not always reactive on template #13525

Closed
adiguba opened this issue Oct 8, 2024 · 13 comments · Fixed by #13542
Closed

[Svelte5] Object are not always reactive on template #13525

adiguba opened this issue Oct 8, 2024 · 13 comments · Fixed by #13542
Assignees
Milestone

Comments

@adiguba
Copy link
Contributor

adiguba commented Oct 8, 2024

Describe the bug

Hello,

I am still confused about the specific behavior of objects in templates.

Since the different functions like set_text(), set_value(), set_attribute compare the value ​​before applying the change.
So objects are only reactive when :

  • They are reassigned.
  • They are concatenated with a string.
  • We call toString() explicitly.

Exemple with a reactive object object :

<!-- This is reactive : -->
<div title="the title : {object}">Text content : {object}</div>
<!-- This is not reactive : -->
<div title={object}>{object}</div>

Reproduction

Click on add to Array to push an element into the array.
Some syntaxes are reactive, others are not...

Logs

No response

System Info

REPL

Severity

annoyance

@paoloricciuti
Copy link
Member

All of this boils down to fine grained reactivity...when calling to string the browser is probably reading length under the hood.

All of them would be reactive if instead of pushing you would reassign the array but since you are only changing some of them you only get updates on the ones that accessed that property.

@trueadm
Copy link
Contributor

trueadm commented Oct 8, 2024

This is working as intended, if you want a reactive objects properties to be reactive, then you need to use toString() like you show in the REPL.

@paoloricciuti
Copy link
Member

I would say we can close this

@paoloricciuti paoloricciuti reopened this Oct 8, 2024
@paoloricciuti paoloricciuti closed this as not planned Won't fix, can't repro, duplicate, stale Oct 8, 2024
@adiguba
Copy link
Contributor Author

adiguba commented Oct 8, 2024

What I find disturbing is that it is not systematic.

<p>a text {object}</p> is reactive, but adding a tag around it like <p>a text <strong>{object}</strong></p> break the reactivity...

@trueadm
Copy link
Contributor

trueadm commented Oct 8, 2024

What I find disturbing is that it is not systematic.

<p>a text {object}</p> is reactive, but adding a tag around it like <p>a text \<strong\>{object}\</strong\></p> break the reactivity...

That's only because we put {object} into a template literal for the whole string, so it's just by chance that it works.

@harrisi
Copy link

harrisi commented Oct 8, 2024

This issue has come up in various places (don't care to find them currently, since the issues tend to be misattributed), which makes it seem like the accidental behavior of having text be reactive without calling .toString() is the expected one. It seems like either the accidental behavior should either always occur (so, not accidental) or never occur so that this isn't a gotcha.

@trueadm
Copy link
Contributor

trueadm commented Oct 8, 2024

I’ll take another look into this tomorrow

@trueadm trueadm reopened this Oct 8, 2024
@trueadm trueadm self-assigned this Oct 8, 2024
@harrisi
Copy link

harrisi commented Oct 9, 2024

I wanted to spend some time to find related issues, since you've offered to spend time considering this.

I quickly found #13054, which is almost identical to by the same author. For the sake of consolidation, I'll add any others I find to that issue, if I find time to do more searching tonight.

@adiguba
Copy link
Contributor Author

adiguba commented Oct 9, 2024

I tried some things and make a fix to set_text() using an internal derived and text value.

export function set_text(text, value) {
	// @ts-expect-error
	if (value !== (text.__t ??= text.nodeValue)) {
+		if (!!text.__d) {
+			// is that enought to destroy derived ?
+			delete(text.__d);
+			delete(text.__s);
+		}

		// @ts-expect-error
		text.__t = value;

+		if (value instanceof Object) {
+			text.__d = $.derived(() => value.toString());
+			text.nodeValue = text.__s = $.get(text.__d);
+		} else {
			text.nodeValue = value == null ? '' : value + '';
+		}
+	} else if (text.__d && text.__s !== $.get(text.__d)) {
+		text.nodeValue = text.__s = $.get(text.__d);
	}
}

=> When the value is an object, text.__d contains a derived for toString(), and text.__s the last value of toString().

However, I don't know how to determine the impact on performance.

Demo here : REPL

  • Comp1 is the Svelte component
  • Comp2 is the same component in JS, hacked to use the new set_text() function.

@adiguba
Copy link
Contributor Author

adiguba commented Oct 9, 2024

Or another solution, using a function and derived for all values :

export function set_text(text, fn) {
	if ($.get(text.__d ??= $.derived(() => (fn())?.toString() ?? '')) !== (text.__t ??= text.nodeValue)) {
		text.nodeValue = text.__t = $.get(text.__d);
	}
}

By changing the call to use arrow functions :

		$.set_text(text, () => $$props.data);
		$.set_text(text_1, () => `data:${$$props.data ?? ""}`);

Demo : REPL

@adiguba
Copy link
Contributor Author

adiguba commented Oct 9, 2024

Another solution (again).

Change the generated code in order to use derived toString() :

+	const text_d = $.derived(() => $$props.data?.toString() ?? '');
+	const text_1_d = $.derived(() => `data:${$$props.data ?? ""}`);
	
	$.template_effect(() => {
-		$.set_text(text, $$props.data);
+		$.set_text(text, $.get(text_d));
-		$.set_text(text_1, `data:${$$props.data ?? ""}`);
+		$.set_text(text_1, $.get(text_1_d));
	});

REPL

=> Sorry from "spam", but I really think it is important to have consistent behavior regardless of the type of variable

@trueadm
Copy link
Contributor

trueadm commented Oct 9, 2024

We don't want to introduce deriveds here, it will greatly increase the memory and overhead for such a simple thing. The only solution I can see is that if the value is not re-assigned that we take it out of the template effect entirely – thus having it work as the others.

@adiguba
Copy link
Contributor Author

adiguba commented Oct 9, 2024

But at compile time the type of the value may not be known.

Maybe throwing a runtime error in this case, eventually only in dev mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment