facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.55k stars 46.78k forks source link

Bug: react got runtime error when user breaks the rules of hooks, instead of telling user what to do #28165

Closed RexSkz closed 8 months ago

RexSkz commented 8 months ago

React version: 18.2.0

Steps To Reproduce

  1. Open the link below, it will show a page with a button.
  2. Click the button, and you will get the error.

Link to code example: https://codesandbox.io/p/sandbox/a-react-hooks-edge-case-that-causes-react-crash-rddmkh?file=%2Fsrc%2FApp.js%3A1%2C1

The current behavior

Users get a runtime error Cannot read properties of undefined (reading 'length').

image

This is because React uses different data structures in memoizedState, e.g. object for useEffect, array for useCallback. If users break the rules of hooks, the following code will crash, which will confuse users:

function updateCallback(callback, deps) {
  ...
    if (nextDeps !== null) {
      var prevDeps = prevState[1];                  // <- 1. prevState is now an object

      if (areHookInputsEqual(nextDeps, prevDeps)) { // <- 2. passes an undefined here
        return prevState[0];
      }
    }
  }
  ...
}

function areHookInputsEqual(nextDeps, prevDeps) {
  ...
    if (nextDeps.length !== prevDeps.length) {      // <- 3. this will crash - prevDeps is undefined
      error('The final argument passed to %s changed size between renders. The ' + 'order and size of this array must remain constant.\n\n' + 'Previous: %s\n' + 'Incoming: %s', currentHookNameInDev, "[" + prevDeps.join(', ') + "]", "[" + nextDeps.join(', ') + "]");
    }
  }
  ...
}

The expected behavior

Users should receive a React message like other scenarios that break the rules of hooks (Rendered more hooks than during the previous render.).

Actually, if we modify the last hook from useCallback to useEffect, we can get this message.

image

ESLint can not identify this scenario and will not report errors. If it's not the responsibility of React, at least the eslint-plugin-react should detect it.

li-jia-nan commented 8 months ago

Mark

c0dedance commented 8 months ago

Mark

gsathya commented 8 months ago

React does track the type of hook called (along with their order): https://github.com/facebook/react/blob/d29f7d973da616a02d6240ea10306a6f33e35ca1/packages/react-reconciler/src/ReactFiberHooks.js#L316-L327

This check should (and does) catch the error when user breaks the rules of react. This is what I see in the console:

Warning: React has detected a change in the order of Hooks called by App. This will lead to bugs and errors if not fixed. For more information, read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks

   Previous render            Next render
   ------------------------------------------------------
1. useState                   useState
2. useRef                     useRef
3. useRef                     useRef
4. useEffect                  useRef
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Arguably there could be a case made for throwing and not just warning on hook type mismatch. I don't know the context here on why we chose to warn, @acdlite do you remember?

EDIT: Found the original PR (https://github.com/facebook/react/pull/14585), but not a lot of context there either. I'm going to close as this is working mostly ok -- we do warn with the correct error.

RexSkz commented 8 months ago

Thanks for your reply. I look through the console and do see this warning message.

There are lots of console messages in my project. When this error occurs, the last few messages in console and the Webpack error overlay are Cannot read properties of undefined, leading me to wonder if it's a React bug... 😂

If this should remain "warning", maybe I'll find another way to let devs notice it.