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

Bug: exhaustive-deps rule doesn't report for useInsertionEffect #31308

Open
luin opened this issue Oct 21, 2024 · 1 comment
Open

Bug: exhaustive-deps rule doesn't report for useInsertionEffect #31308

luin opened this issue Oct 21, 2024 · 1 comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@luin
Copy link

luin commented Oct 21, 2024

React version: 18
eslint-plugin-react-hooks version: 4.6.2

Steps To Reproduce

Run eslint. with eslint-plugin-react-hooks with the following two code:

  1. useEffect:
const App = ({ id }: { id: string }) => {
  useEffect(() => {
    document.title = id;
  }, []);
};
  1. useInsertionEffect:
const App = ({ id }: { id: string }) => {
  useInsertionEffect(() => {
    document.title = id;
  }, []);
};

The current behavior

The plugin reports a warning for the first case and doesn't for the second.

The expected behavior

For both cases the plugin should report a warning.

As a workaround, I currently have to update the config:

        'react-hooks/exhaustive-deps': [
          'warn',
          {
            additionalHooks: 'useInsertionEffect',
          },
        ],
@luin luin added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Oct 21, 2024
@glyph-cat
Copy link

glyph-cat commented Jan 3, 2025

This has been bothering me for a while too. Thank you for opening this issue, this was the last push that I needed.

I finally decided to dig through the code and I found that it was only missing one line of code in ExhaustiveDeps.js:

switch (node.name) {
case 'useEffect':
case 'useLayoutEffect':
case 'useCallback':
case 'useMemo':
// useEffect(fn)
return 0;

 switch (node.name) { 
    case 'useEffect': 
    case 'useLayoutEffect': 
+   case 'useInsertionEffect': 
    case 'useCallback': 
    case 'useMemo': 
      // useEffect(fn) 
      return 0; 

I have created PR #31969 (tests included), which should address this issue and is currently awaiting for review. Do let me know if you have any additional thoughts on this in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants