Skip to content

Svelte 5: reactive date may be not reactive in the markup #10629

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

Closed
7nik opened this issue Feb 24, 2024 · 15 comments
Closed

Svelte 5: reactive date may be not reactive in the markup #10629

7nik opened this issue Feb 24, 2024 · 15 comments

Comments

@7nik
Copy link
Contributor

7nik commented Feb 24, 2024

Describe the bug

I was told that <div>{reactiveDate}</div> being non-reactive is a bug.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA12QwWrDMBBEf2URBdskrWy3vTi2odA_6LHpwZXXiaglGWudEoT-vZKTNlDQZXbf7AxybJAjWla9O6Y7haxiL9PEtozOUxT2hCNh0NYss4iT2opZTtTu9Z6kmsxM4OC1IwQPw2wUJBcPn7ETJE-SzskuwAEfkaCPZAMav1dTmpR5-cTzkpePUDxXeR5eku1-cavoGPA7SxGO85rfCui6l6dWG31_DcMKXEzwNY-bK_B_uQVrFNJR6gPgaBFcjPnzBNfnQmQ0GC1GKb4al2bQtOBiq3jhwSK9oTC6t-mqDzedwQaKWNT7dlOAXcc1v1xsw08q08tBYs8qmhf0H_4Hwxyqc4QBAAA=

Logs

No response

System Info

Svelte v5.0.0-next.68

Severity

annoyance

@brunnerh
Copy link
Member

This would work when explicitly calling .toString().

Svelte uses the internal function stringify which forces the object to become a string via value + ''.
I thought that maybe defining Symbol.toPrimitive on the reactive date might help but that's not quite it, even though it is called initially. Rather strange...

@dummdidumm
Copy link
Member

I'd say this works as expected given the inner rules of the system, even if it's confusing. Doing static analysis to see that date is coming from new Date() feels wrong because you can't statically analyze all cases where this happens. When having export const date = new Date(..) in a .js file an importing that then the import wouldn't be treated as reactive.

Maybe calling .toString() on all variables would help, but not sure that bundle size overhead is worth it.

@Conduitry
Copy link
Member

Yeah I think I'd be in favor of documenting that this is how this works and then moving on. Doing static analysis sounds bad (this is supposed to just be a blessed implementation of something users could also do themselves) and adding toString()s everywhere also sounds bad.

@Prinzhorn
Copy link
Contributor

How is it that the (in my opinion) most important use-case for dates works though?

<div>non-reactive: {date}</div>
<div>this works: {format(date)}</div>

It doesn't call a method on date so it's really weird that this gets invalidated but plain {date} is not? How does that make sense even when you document it? Without understanding anything about what you said above, why can Svelte invalidate the second case but not the first?

@Conduitry
Copy link
Member

What does the format function do in your example? Does it call methods on the object?

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Feb 24, 2024

Looking at the OG REPL again, you all might have missed the most important bit. This works:

<div>reactive: {date}, something else {smth}</div>

so it's clearly an issue of tracking reactivity for date when adding smth suddenly makes it work, no?

@7nik
Copy link
Contributor Author

7nik commented Feb 24, 2024

The compiler doesn't consider date as a reactive variable and therefore compiles <div>non-reactive: {date}</div> as non-reactive. But <div>reactive: {date}, something else {smth}</div> contains reactive smth, so it is compiled this block as reactive markup and it subscribes to both date and smth.

Also, https://discord.com/channels/457912077277855764/1153350350158450758/1210733443189579896 here I was told this behaviour is a bug.

@stalkerg
Copy link
Contributor

I'd say this works as expected given the inner rules of the system, even if it's confusing.

in that case, maybe rules is wrong, no?
if it's not working as expected even for proxy object which was imported, it's wrong.

@7nik
Copy link
Contributor Author

7nik commented Feb 25, 2024

A bit more details for those who aren't familiar with Svelte 5 nuances.
AFAIK Svelte 5, a markup expression is considered non-reactive when, firstly, it is a bare variable (e.g. {foo} but not {foo.bar}, {foo.method()}, {foo + bar} and so on), and, secondly, the variable is non-reactive (declared without using $state, $derived or $props).
Another nuance is optimisation by grouping multiple text updates into one operation: text nodes are re-calculated entirely, so, e.g. in <div>{Math.random()} some text {foo}</div> the random value will change when foo changes. It causes re-running expressions that didn't change, but a neighbour expression did.
When these two nuances overlap, a behaviour like this issue may occur. However, the compiler tries to display a warning in such cases.


It seems Dominic considers this a bug, and a variable should be considered reactive when it's initialized with Svelte's reactive class.


My opinion is it's very doubtful somebody will print Date as-is: the default date format isn't really convenient for the end user, and dates almost always get a custom format or at least .toLocaleString() / .toLocaleDateString() / .toLocaleTimeString().

About this issue in general: I think the text node rendering optimisation is an implementation detail, and a programmer shouldn't care about it or even be aware of it. Probably, non-reactive markup expressions should be memorized to provide consistent behaviour across all text nodes.
However, I wonder how big the overhead is if the definition of non-reactive markup expression will be narrowed to only bare constants.

@brunnerh
Copy link
Member

My opinion is it's very doubtful somebody will print Date as-is: the default date format isn't really convenient for the end user, and dates almost always get a custom format or at least .toLocaleString() / .toLocaleDateString() / .toLocaleTimeString().

You can also manually call .toString() to make it reactive again.

@trueadm
Copy link
Contributor

trueadm commented Feb 26, 2024

I feel like we should be using static analysis here to know that the binding itself is from Date. This has nothing to do with toString() but more the fact that we don't consider the binding as reactive.

@brunnerh
Copy link
Member

That is not quite accurate, toString is called implicitly (REPL) and the combination of that triggering the internal signal and a text_effect being compiled into the output causes the updates.

I doubt that static analysis is reliable, the value could be imported from somewhere else etc.

@trueadm
Copy link
Contributor

trueadm commented Feb 26, 2024

Yeah, let's close this then. The false positives aren't worth it as per #10636.

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.

7 participants