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

Do not inject setState/ref into dependencies #10

Closed
Glinkis opened this issue Mar 26, 2020 · 7 comments
Closed

Do not inject setState/ref into dependencies #10

Glinkis opened this issue Mar 26, 2020 · 7 comments

Comments

@Glinkis
Copy link

Glinkis commented Mar 26, 2020

Currently testing this macro, so far with great success!

But I have one minor nitpick.

Currently this:

const [state, setState] = useState(0)
useAutoEffect(() => {
   setState(1)
})

Is converted to:

const [state, setState] = useState(0)
useEffect(() => {
   setState(1)
}, [setState])

But adding setState to the dependency array is redundant, since react guarantees it to be referentially stable. The same is true for useRef.

The react-hooks/exhaustive-deps eslint takes both of them into consideration.

@yuchi
Copy link
Owner

yuchi commented Mar 27, 2020

This is a very nice suggestion! We could also exclude:

  1. the state value if the setter is never used
  2. memoized values/callbacks that use an empty dependencies array

@Glinkis
Copy link
Author

Glinkis commented Mar 27, 2020

Thanks for the response!
I'm not sure I quite understand your suggestions. It would be awesome if you could show an example of each of them.

@yuchi
Copy link
Owner

yuchi commented Mar 27, 2020

function MyComponent() {
  const [id] = React.useState(() => Math.random());

  // Doesn’t require [id] since it will never change
  useAutoEffect(() => {
    console.log(id);
  });

  const [counter, setCounter] = React.useState(0);

  // Again, `setCounter` is not used, `counter` will never change!
  const values = useAutoMemo(new Array(counter));

  const handleLog = React.useCallback(() => {}, []);

  // `handleLog` will (*should*) never change, using as a dependency is reduntant
  useAutoEffect(() => handleLog('Hallo!'));
}

@yuchi
Copy link
Owner

yuchi commented Apr 6, 2020

Found the relative code in the ESLint exhaustive-deps rules: https://github.com/facebook/react/blob/fe2cb525542443aaf1447c4069354c12659fd186/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L296

Should be straight forward to add them to this macro.

@yuchi
Copy link
Owner

yuchi commented Apr 6, 2020

Please see #11 and #12 for further discussion on those additional optimizations.

@yuchi yuchi closed this as completed in f0082d3 Apr 6, 2020
@yuchi
Copy link
Owner

yuchi commented Apr 6, 2020

@Glinkis you can see this implemented in v1.1.0. Should bring those small perf improvements :)

@Glinkis
Copy link
Author

Glinkis commented Apr 6, 2020

Thanks!

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