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

SvelteDate reactivity issue #13513

Closed
felix-roehrich opened this issue Oct 5, 2024 · 15 comments · Fixed by #13515
Closed

SvelteDate reactivity issue #13513

felix-roehrich opened this issue Oct 5, 2024 · 15 comments · Fixed by #13515
Assignees
Labels
Milestone

Comments

@felix-roehrich
Copy link

felix-roehrich commented Oct 5, 2024

Describe the bug

In a previous issue #12717, I discovered a caching problem which was fixed in 210. Now after updating I am having a similar issue. After some trials, this seems to have been introduced from 246 to 247 and exists in the latest 262.

Reproduction

  1. Open the REPL and click the button. The text in the button should become unresponsive after a few clicks.
  2. The issues disappears when the {date.getMonth() == 1} expression is moved away from the class:, e.g. having the expression as text will not trigger the issue.
  3. Having just {date} as the button text is always unresponsive (This is minor caveat I discovered when trying to reproduce the issue.)

Logs

-

System Info

REPL

Severity

blocking an upgrade

@felix-roehrich
Copy link
Author

Most likely #13171 is the culprit.

@paoloricciuti
Copy link
Member

Most likely #13171 is the culprit.

I don't think that's the issue...i'm checking and it seems like it's setting the value to the same time. I'm investingating what's this all about

@paoloricciuti paoloricciuti self-assigned this Oct 6, 2024
@paoloricciuti
Copy link
Member

The problem is not with the class: part but generally with the fact that is accessed twice

REPL

@felix-roehrich
Copy link
Author

Interesting, but in class directive it is only accessed once(?).

@paoloricciuti
Copy link
Member

Interesting, but in class directive it is only accessed once(?).

Yeah there's definitely something else going on but i don't think it's related to that change. I'm investigating tho.

@paoloricciuti
Copy link
Member

Mmm ok the problem with my repl is kinda the same as the class: one...those calls are treated as deriveds and that's why it doesn't work. So the problem was indeed introduced in that PR but it's not in the PR itself...it just surfaced the problem.

@paoloricciuti
Copy link
Member

I think i need to surrender to this one and invoke @trueadm ...from my exploration the problem seems to be that the derived returned from getMonth() is marked as destroyed after the second click and that seems to be is what is causing the issue. I will try to continuing exploring what is destroying the derived but it's getting tangled 😄

@paoloricciuti
Copy link
Member

This is a simpler (?) reproduction

The intermediate derived is probably destroying the children whilst being updated.

@adiguba
Copy link
Contributor

adiguba commented Oct 6, 2024

Hello,

Even stranger : just adding a count.getMonth() on the script "fix" the bug !!!

REPL

@paoloricciuti
Copy link
Member

I think I kinda figure out what's going on. In SvelteDate we are creating the derived inside the methods. This is problematic if the first thing to call the method is a derived itself because that derived will be destroyed by the parent derived.

I literally just figure out this on my phone so I might be wrong and I still can't fix it but I will investigate better

@paoloricciuti
Copy link
Member

Hello,

Even stranger : just adding a count.getMonth() on the script "fix" the bug !!!

REPL

And this also kinda confirm my theory...because now the derived is not created inside the derived so it's just "a normal derived"

@adiguba
Copy link
Contributor

adiguba commented Oct 6, 2024

I logged the derived and it's flags, and your right : after the click the derived inside SvelteDate is marked as DESTROYED.

@paoloricciuti
Copy link
Member

I logged the derived and it's flags, and your right : after the click the derived inside SvelteDate is marked as DESTROYED.

Yup...which btw is perfectly fine because in userland you can't keep a reference to the derived signal outside of the derived itself so when the derived is invalidated it's fine to destroy each children. However in SvelteDate we are keeping a reference in a Map and that's where it breaks

@paoloricciuti paoloricciuti added this to the 5.0 milestone Oct 6, 2024
@trueadm
Copy link
Contributor

trueadm commented Oct 6, 2024

We need to check if the derived in the map has the destroyed flag and if so, create it again

@paoloricciuti
Copy link
Member

We need to check if the derived in the map has the destroyed flag and if so, create it again

Setting the current reaction to be the parent of the reaction before creating the derived fixes the problem but I don't know if this has some problem. But this solution seems like a good solution, let me try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants