React: 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?

```js
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.

[Dergash]

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

I believe the "top level" phrasing is a two-sided sword: from one side, it is a simple concept to remember and explain to other developers, but from the other side, it can get in a way of understanding the reasons behind this rule (which are of course explained below the title).

According to those reasons, it is important to preserve the order and number of hook calls between the render. This is achieved by rules of prohibiting things that may change the order and the number of hooks.

In the solution that @audiolion proposed he avoids changing both, because _all_ of the hook calls are placed after return statement, so they will never change. In fact I think this is what had to be done if you need to optimise things in case your hooks are computationally expensive and you don't want them to fire if you will hit early return. So it can placed before or after, but if you mix it (before and after) then you break the rule

[nickretallack]

@Dergash are you saying @audiolion is correct? Because he's not. Try running eslint on his solution and you'll get this:

   6:3  error    React Hook "useEffect" 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?  react-hooks/rules-of-hooks

Just because you put if (!isUserId || !isHovering) return null; inside a hook doesn't make it not an early return. The number of hooks that get called will differ if values change such that the early return happens or does not. That's what this thread is all about.

[nickretallack]

Here's an example of the solution I'm proposing for working around this.

Original component that breaks the rule:

export default ({shouldRender}) => {
  if (!shouldRender) return null;
  React.useEffect(() => console.log("hi");
  return "Hello"
}

New code that works around the lint rule:

// actual component is here
const ActualComponent = () => {
  React.useEffect(() => console.log("hi");
  return "Hello"
}

// tiny wrapper component whose only purpose is to conditionally exit early
export default ({shouldRender}) => {
  if (!shouldRender) return null;
  return <ActualComponent/>
}

I often have to create tiny wrapper components like this for other reasons, anyway, such as exploiting the key property to cause re-renders.

[Dergash]

@nickretallack

You are right about putting early return inside the hook not being valid solution, I was wrong here and overall I'm inclined to agree that putting hooks before any early return is a correct way.

Though there is some background on why I've answered the way I've answered initially an I'd like to share it.
Check this code example: https://codesandbox.io/s/fancy-star-g95r2
This is a pattern that I observe in our codebase a lot:

const Child = props => {
  if (props.earlyReturn) {
    return <div>We are inside early return</div>;
  }
  const [index, setIndex] = React.useState(0); // eslint-disable-line

As you can see, although there is an obvious eslint warning on this pattern, it does not crash the application and does not break the logic, which is why I've assumed that putting it inside hook as suggested by @audiolion will work.
I did not check the internals yet but it seems that my logic might have some basis afterall in the way that hooks ordering check occurs during on the hook call, not on the render per se, so if your first render has two hooks and second has one this will crash as soon as the hook from the second render is called, but if your first render have two hooks and the second have none at all than it will not crash

Please correct me if I'm wrong again :)

P.S. Wrapper component is one way of handling this but the whole idea behind the hooks was to decrease the level of nesting for components, and wrappers are not free in terms of resources and perfomance.

[nickretallack]

@Dergash no that will still crash you in development mode when you go from the non-early exit version to the early-exit version of the component, regardless of how many hooks you have in it. I must admit I've seen code like that in my project too and it didn't crash, but it could crash if the component ever went back to the early exit version I think.

TBH I don't see any way around this other than the wrapper component strategy. It's a necessity because that's how react organizes hooks: by component. I'm not terribly concerned about this adding overhead. Should I be? This is JavaScript. It's fine. Usually we should look at memoizing things to prevent needless re-renders first anyway, right?

[zzdjk6]

Apart from spreading and duplicating null checks in hooks, I think we could use render props as a solution which also gives us a reusable and composable component-level abstraction.

For example, imagine that we want to do something like this (which is not valid):

const MyComponent: React.FC = () => {
  const data1 = useData1();
  if (!data1) {
    return null;
  }

  const data2 = useData2(data1);
  if (!data2) {
    return null;
  }

  const result = React.useMemo(() => aggregateData(data1, data2), [
    data1,
    data2,
  ]);

  if (!result) {
    return null;
  }

  return <ResultDisplay result={result} />;
};

We could create some DataGuard components:

type Data1GuardProps = {
  children: (data1: Data1) => JSX.Element;
};
const Data1Guard: React.FC<Data1GuardProps> = ({ children }) => {
  const data1 = useData1();
  if (!data1) {
    return null;
  }

  return children(data1);
};

type Data2GuardProps = {
  data1: Data1;
  children: (data2: Data2) => JSX.Element;
};
const Data2Guard: React.FC<Data2GuardProps> = ({ data1, children }) => {
  const data2 = useData2(data1);
  if (!data2) {
    return null;
  }

  return children(data2);
};

And then we should be able to compose them like this:

const MyComponent: React.FC = () => {
  return (
    <Data1Guard>
      {(data1) => (
        <Data2Guard data1={data1}>
          {(data2) => (
            <AggregateDataGuard data1={data1} data2={data2}>
              {(result) => <ResultDisplay result={result} />}
            </AggregateDataGuard>
          )}
        </Data2Guard>
      )}
    </Data1Guard>
  );
};

These components can be used anywhere needs them regardless what presentational layer is used as they do not emit DOM directly by themselves.