Rules of Hooks don't support top-level exit conditions

[fabiospampinato]

Do you want to request a feature or report a bug? bug

What is the current behavior?

The following piece of code breaks the rules of hooks:

import {useState} from 'react';
const MyComponent = () => {
  const [foo, setFoo] = useState ();
  if ( !foo ) return;
  const [bar, setBar] = useState ();
  return null;
};

The lint rule reports:

React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?

What is the expected behavior?

My understanding is that for hooks to work they need to be called always in the same order, otherwise returned values can't be properly mapped to function calls.

In my piece of code the hooks are always called in the same order, but some times we exit early, which is slightly beneficial both for performance and for writing clean code.

Shouldn't this just work?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React? v16.8.6

[audiolion]

when you are exiting early, you aren't calling them in the same order, sometimes when the component renders it has 1 hook because it returns early, other times you render and it has 2 hooks because you didn't return early.

I might be wrong about this, but I think react is making assumptions about the component saying this component will always have 2 hooks when it renders, but you have written it such that this assumption becomes an invariant error

[fabiospampinato]

sometimes when the component renders it has 1 hook because it returns early, other times you render and it has 2 hooks because you didn't return early.

Yeah but they are still in the same order, it's impossible for the second useState call to occur before the first one.

I might be wrong about this, but I think react is making assumptions about the component saying this component will always have 2 hooks when it renders, but you have written it such that this assumption becomes an invariant error

I've re-read the docs about this and I see no mention about preserving the same number of hooks calls, the rules seem to be: call hooks at the top-level (✅) and call them always in the same order (✅), so this sounds like a bug to me 🤔.

[jquense]

You do need to maintain the same number of hooks as part of preserving the order. The docs aren't super clear on this particular case but it's really covered by "Don't call hooks conditionally" which is what's happening here

Note, that this isn't just eslint React itself will yell at you if you try this: https://codesandbox.io/s/cranky-lamarr-k2fc6

[fabiospampinato]

@jquense right 🤔 I see the detected error is:

React has detected a change in the order of Hooks called by MyComponent

Which sounds a bit off to me for this particular scenario.

Is there a technical reason why these sorts of top-level exit conditions couldn't be supported?

[gaearon]

We could in theory support them. However we’ve chosen intentionally against that because semantics get very confusing.

What would happen to state below? Would it be reset or preserved? What about effects? Would they be cleaned up (like on unmount) or would they run normally?

If you think about it you’ll find cases that are very confusing regardless of the behavior you pick. Therefore, we disallow this pattern altogether. Put early return after calls to Hooks.

[togakangaroo]

So to comment on this further, this oddly enough causes more usage of HoCs. For example I have a component <DisplayUserinfo userId /> which is called inside a ReactHoverObserver and internally does a bunch of loading of data by user id.

So before updating to latest with the rule I just had

const DisplayUserInfo = ({isHovering, userId}) => { 
   if(!isHovering || !isUserId)
     return null
   ... load and render stuff ...

this worked fine because the ordering is preserved. However now I have to do

const DisplayUserInfo = withHoveringTrue(withUserId(({userId}) => {
  ...

just to get the linter to pass.

[audiolion]

@togakangaroo what if you move those conditions into the hooks themselves?

const DisplayUserInfo = ({ isHovering, userId }) => {
  const fetchUser = useFetchUser()
  React.useEffect(() => {
    if (!isHovering || !isUserId) return;
    fetchUser();
  }, [fetchUser]);
  
  if (!isHovering || !isUserId) return null;

  // return component
};

[togakangaroo]

@audiolion

does a bunch of loading of data by user id

So I have...oh...let's say 4 requests that go out. It's a hover-over "drill down into details" sort of action so I'm not too worried about having too many requests, and the data pops into the UI as it becomes available. I would then have to duplicate this logic for every single one of those data requests.

Worse, in reality I would of course be abstracting these out into custom hooks that handle the specifics of data fetching for my particular application - would I then have to have my functions that have to do with loading data know about an isHovering property? Messy...

I have not looked at the hooks source code, but I have written something pretty much identical to useState so I assume (and maybe I'm wrong?) I know how it works. As @fabiomcosta said, it's the order that matters, not whether all of them fire. If its possible to lint for that then we should. Otherwise just be aware, it increases the usage of HoCs.

[fabiomcosta]

@togakangaroo you might have mentioned the wrong person 😂

[audiolion]

EDITED 2020 -- as others pointed out my original comment violated rules of hooks, woops! edited to be correct.

@togakangaroo if there are multiple requests you can wrap them in your own hook:

const useHoverOver = ({ isUserId, isHovering }) => {
  const [state, setState] = React.useState()
  useEffect(() => {
    if (!isHovering || !isUserId) return
    // fetch 1
    // fetch 2
    // fetch 3
    // fetch 4
    setState({
      ...fetch1Data,
      ...fetch2Data,
      ...fetch3Data,
      ...fetch4Data,
    })
  }, [isHovering, isUserId])

  return state
}

const DisplayUserInfo = ({ isUserId, isHovering }) => {
  const data = useHoverOver({ isUserId, isHovering });
  if (!data) return null;
  // return my component
};

hooks can be composed just like HOCs, so each fetch could be a custom hook with handling logic, or you could just put it all together into one hook that you re-use.

Similarly you could move the is checks outside the component:

return (
  <>
    {isUserId && isHovering && <DisplayUserInfo />}
  </>
);

And make the user of the component not render unless those variables are present.

My point is: while the rules of hooks do limit us from some JS idioms, I think there are still options for composing custom hooks to do the same thing or changing the way you consume the component.

[nickretallack]

@audiolion isn't that breaking the rules of hooks too? You're still conditionally calling them inside your custom hook.

Honestly I think the simplest work-around to this is to create a higher order component that decides if it should render its child or null. Still, I wish the lint rule would allow you to avoid this sort of work-around.