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

Re-assigning NaN is seen as a value change #614

Open
2 of 5 tasks
Legioth opened this issue Oct 23, 2024 · 3 comments
Open
2 of 5 tasks

Re-assigning NaN is seen as a value change #614

Legioth opened this issue Oct 23, 2024 · 3 comments

Comments

@Legioth
Copy link

Legioth commented Oct 23, 2024

  • Check if updating to the latest version resolves the issue

Environment

  • I am using @preact/signals-core
  • I am using @preact/signals
  • I am using @preact/signals-react

Describe the bug
In JavaScript, NaN === NaN evaluates to false. A consequence of this is that a signal with the value NaN will trigger an update if the value is again assigned as NaN. In the worst case, this might lead to an infinite loop with a two-way UI binding if the receiving end is also treating NaN in the same way.

To Reproduce

const x = signal(0);
effect(() => console.log(x.value));
x.value = NaN;
x.value = x.value;

This logs NaN twice even though it's expected that the last row shouldn't lead to running the effect again.

Expected behavior
Expected that the dirty checking logic has a special case for re-assigning NaN

@rschristian
Copy link
Member

I don't want to say this won't be supported here, but I'd really, really recommend against setting NaN in any state value. It's essentially an unhandled error, keeping it around is usually a mistake. Better to fallback to -1 or gracefully handle it in a similar form.

@Legioth
Copy link
Author

Legioth commented Oct 24, 2024

Just for context, I discovered through through doing parseInt(fieldValue) for an empty string and then wasted some debugging time looking at other things before remembering that NaN != NaN. More graceful handling in the signal would have helped me avoid an infinite loop which would in turn have made it easier for me to realize that I had an unexpected "number" in the system.

@rschristian
Copy link
Member

rschristian commented Oct 24, 2024

You should really never call parseInt without having a fallback though is my point, which is more of a thing for a linter to solve (I seem to recall there being a rule for this to, but not positive).

I believe the intention here is to still ship ES5, so Object.is() is likely out, and anything else would probably hurt perf slightly. We also don't support it in Preact for that reason.


Again, not saying we won't, but I at least would prefer not to. It's a bad pattern to pass NaN around, IMO.

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

No branches or pull requests

2 participants