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

Handle NaN comparison in useEffect dependency array #3954

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jayrobin
Copy link

Summary

When comparing changes to useEffect dependencies, if any dependency is NaN it will result in the effect always being run, even if the value was NaN on the previous render. This is due to NaN !== NaN in JavaScript.

This change handles the NaN case using the is utility introduced in #3776. Presumably we don't want to use Object.is due to incompatibility with IE11 (thanks for the insight @andrewiggins!). Note that I copied the is utility, which is also present in compat/src, but I'm happy to raise it up to src/utils and use it in both places if that's preferred.

Issue

Note: this will cause Preact to hang due to infinite re-renders

import { render } from 'preact';
import { useState, useEffect } from 'preact/hooks';

function Component({ value }) {
  const [count, setCount] = useState(0);
  useEffect(() => {
    setCount(count + 1);
  }, [value]);
  return <p>{count}</p>;
}

function App() {
  const [value, setValue] = useState(undefined);
  setValue(NaN);

  return <Component value={NaN} />;
}

render(<App />, document.getElementById('app'));

Expected behavior
Preact runs the effect once, because value never changes from NaN.

Actual behavior
Preact re-renders forever, because it strictly compares NaN to NaN.

@coveralls
Copy link

coveralls commented Mar 30, 2023

Coverage Status

Coverage: 99.702% (+0.0004%) from 99.701% when pulling d421929 on jayrobin:jmr--fix-nan-comparison-useeffect into 59f7f7c on preactjs:master.

@andrewiggins
Copy link
Member

andrewiggins commented Mar 30, 2023

Size Change: +90 B (0%)

Total Size: 54.4 kB

Filename Size Change
hooks/dist/hooks.js 1.56 kB +31 B (1%)
hooks/dist/hooks.module.js 1.59 kB +28 B (1%)
hooks/dist/hooks.umd.js 1.65 kB +31 B (1%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.91 kB 0 B
compat/dist/compat.module.js 3.84 kB 0 B
compat/dist/compat.umd.js 3.98 kB 0 B
debug/dist/debug.js 3 kB 0 B
debug/dist/debug.module.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 232 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
dist/preact.js 4.21 kB 0 B
dist/preact.min.js 4.24 kB 0 B
dist/preact.min.module.js 4.24 kB 0 B
dist/preact.min.umd.js 4.27 kB 0 B
dist/preact.module.js 4.23 kB 0 B
dist/preact.umd.js 4.27 kB 0 B
jsx-runtime/dist/jsxRuntime.js 360 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 326 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 441 B 0 B
test-utils/dist/testUtils.js 442 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 526 B 0 B

compressed-size-action

@jayrobin
Copy link
Author

I've rolled in the changes from the draft PR #3955, which used the same approach to fix a similar bug in useState.

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 this pull request may close these issues.

None yet

3 participants