testing-library/react-testing-library

Enforce consistent hook order for all hooks, including `useContext`

12joan opened this issue · 4 comments

Describe the feature you'd like:

When calling the rerender method returned by renderHook, it would be useful for the library to throw an error like React does if the order of native React hooks called is not the same between renders.

In practice, this is already the case for some hooks like useMemo and useState whose implementation is dependent on order by necessity, but it is not the case for useContext, for example.

const MyContext = createContext(null);

let firstRender = true;
const useMyHook = () => {
  if (!firstRender) {
    useContext(MyContext);
  }
  firstRender = false;
};

// Expected: An error is thrown due to inconsistent hook order
// Actual: No error is thrown
renderHook(useMyHook).rerender();

Describe alternatives you've considered:

When writing idiomatic React code, this case should not arise. However, library code often needs to bend the rules, and having a test suite that ensures we're doing so safely would make it much easier to catch bugs.

Teachability, Documentation, Adoption, Migration Strategy:

In some niche circumstances where the use of hooks in test code differs from their use in production, this may be a breaking change for some users, especially if the implementation of the hook order check differs from React's implementation in some cases. For this reason, I would recommend adding an option to bypass the hook order check.

This is best filed against facebook/react. React Testing Library cannot detect conditional rendering of Hooks.

useContext working conditionally is an implementation detail of React and shouldn't be relied upon.

@eps1lon Apologies if I've misunderstood something, but I don't think there's any issue with React itself. React does not support calling useContext conditionally, but React Testing Library does. This means that React Testing Library can fail to catch issues that will be errors when the code is run using React.

For example, in the following code sandbox, React logs a warning in the console.
https://codesandbox.io/p/sandbox/usecontext-hook-order-gm6fd6?file=%2Fsrc%2FApp.tsx%3A25%2C1

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. useReducer                 useReducer
2. useMemo                    useContext
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
image.mp4

Is there a technical reason that makes it impossible for React Testing Library to detect this?

React does not support calling useContext conditionally, but React Testing Library does.

It's probably just some timing of renders. React Testing Library is not doing something exotic here. It's really just a minimal wrapper around React's APIs.

Can you write a test reproducing the steps you take in the codesandbox and check if there really is no warning when RTL is used?

React Testing Library is not doing something exotic here. It's really just a minimal wrapper around React's APIs.

Ah, I see. I had assumed React Testing Library would need to use custom implementations for React's hooks, but I see now from looking at the code that this isn't the case.

I extended the code sandbox to use React Testing Library both in the browser and in a Jest test, and it turns out that the warning does show up in all environments. https://codesandbox.io/p/devbox/kwyr44?file=%2Fsrc%2FApp.tsx%3A28%2C11

  console.error
    Warning: React has detected a change in the order of Hooks called by TestComponent. 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. useRef                     useRef
    2. useEffect                  useContext
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
        at TestComponent (/project/workspace/node_modules/.pnpm/@testing-library+react@16.2.0_@testing-library+dom@10.4.0_@types+react-dom@18.2.15_@types+rea_m2uv4nqfiyzlepw2flqgd5b5zi/node_modules/@testing-library/react/dist/pure.js:323:5)

      11 |   if (!isFirst.current) {
      12 |     console.log("useContext");
    > 13 |     useContext(MyContext);
         |               ^
      14 |   }
      15 |
      16 |   isFirst.current = false;

I'm not sure why the warning was suppressed when we encountered this issue in our project, but there certainly doesn't seem to be any issue with React Testing Library. Sorry for wasting your time.