Eslint-plugin-react: Rule proposal: react/no-unstable-nested-components

Created on 9 Aug 2020 · 17Comments · Source: yannickcr/eslint-plugin-react

I'd like to introduce a new rule for detecting React components which are declared inside another component or inside class component's render block. I was surprised to see this rule doesn't exist yet.

I've named this rule react/no-unstable-nested-components - if anyone can come up with a better name I'm open for it.

I'll set up the PR soon. Here's the implementation AriPerkkio/eslint-plugin-react/tree/react/no-unstable-nested-components.

Use case

Prevent developers from creating components inside another components.

1. Performance

function Component() {
  function UnstableNestedComponent() {
    return <div />;
  }

  return (
    <div>
      <UnstableNestedComponent />
    </div>
  );
}

When Component re-renders (e.g. parent state updates) the UnstableNestedComponent is recreated. React reconciliation will compare the previous render's UnstableNestedComponent and the current one using reference equality. Since the reference does not match, the element will be completely recreated - not to be confused with re-rendering. I guess this is a small performance optimization, especially in cases where this pattern is used on application root level.

Since all the children of the nested component are also recreated on each re-render, optimization provided by React.PureComponent is gone. The PureComponent is recreated on each re-render.

class PureComponent extends React.PureComponent {
  render() {
    return <div />;
  };
};

function Component(props) {
  function Wrapper() {
    return <div>{props.children}</div>;
  };

  return (
    <div>
      <Wrapper />
    </div>
  )
};

...

<Component>
  <PureComponent />
</Component>

2. Prevent unnoticeable bugs

This is the most important use case of this rule. In addition to performance optimization this rule will prevent possible bugs.

In cases where the nested component is rendering given stateful children, each re-render will reset the children's state back to the initial state. Without this rule these cases are easily unnoticeable and cause weird behavior in apps.

function Accordion(props) {
  const [isOpen, toggle] = React.useReducer(s => !s, false);

  return (
    <>
      <button onClick={toggle}>Toggle</button>
      <div style={{ opacity: isOpen ? 1 : 0 }}>
        {props.children}
      </div>
    </>
  );
};

function Menu(props) {
  function MenuWrapper() {
    return <div>{props.children}</div>;
  };

  return (
    <div>
      <h2>Menu</h2>
      <MenuWrapper />
    </div>
  );
};

export default function Navigation() {
  return (
    <nav>
      <button onClick={React.useReducer(() => [], [])[1]}>
        Force render
      </button>
      <Menu>
        <Accordion>About us</Accordion>
        <Accordion>Contact</Accordion>
      </Menu>
    </nav>
  );
};

In example above:

Click About us to open the "accordion"
Click force render to update parent state and to trigger re-render

Expected: Accordion stays open
Actual: Accordion closes. Component is recreated and initial state is set.

Rule Details

The following patterns are considered warnings:

function Component() {
  function UnstableNestedComponent() {
    return <div />;
  }

  return (
    <div>
      <UnstableNestedComponent />
    </div>
  );
}

class Component extends React.Component {
  render() {
    function UnstableNestedComponent() {
      return <div />;
    }

    return (
      <div>
        <UnstableNestedComponent />
      </div>
    );
  }
}

The following patterns are not considered warnings:

function OutsideDefinedComponent(props) {
  return <div />;
}

function Component() {
  return (
    <div>
      <OutsideDefinedComponent />
    </div>
  );
}

function Component() {
  const MemoizedNestedComponent = React.useCallback(() => <div />, []);

  return (
    <div>
      <MemoizedNestedComponent />
    </div>
  );
}

Rule Options

...
"react/no-unstable-nested-components": "off" | "warn" | "error"
...

Source

AriPerkkio

👍10

All 17 comments

While I agree with the principle of this rule, component detection is very difficult, and this will likely result in a lot of false positives.

ljharb on 9 Aug 2020

I'm starting to realize the difficulty of component detection now. At first I though the Components.detect(.. would detect most cases, but no.

I've ran this linter to material-ui, ant-design and some other big repositories. I still need to add rule for detecting components declared inside props, e.g. <Component footer={() => <div />} />.

Also it seems that renderProps have to be excluded from the rule. I'm not exactly sure how React is handling the renderProps but for some reason they are not causing the _children losing state_ issue.

function Menu(props) {
    const Footer = props.footer;
    return (
        <div>
            <h2>Menu</h2>
            {props.children({})}
            <Footer />
        </div>
    );
};

export default function Navigation() {
    return (
        <nav>
            <button onClick={React.useReducer(() => [], [])[1]}>
                Force render
        </button>
            <Menu footer={() => <Accordion>Footer</Accordion>}>
                {(renderProps) => {
                    return (
                        <>
                            <Accordion>About us</Accordion>
                            <Accordion>Contact</Accordion>
                        </>
                    );
                }}
            </Menu>
        </nav>
    );
};

In the example above only Footer will hide when force render is clicked. Components inside renderProps remain their state.

AriPerkkio on 12 Aug 2020

render props aren't components, and are usually called, not used in an element context, so they don't have any state.

ljharb on 12 Aug 2020

👍1

_called_, not used in an element context.

Thanks, that was exactly the case here. The returned element will always be the same React.Fragment as _a stable reference_.

After adding detection for component inside prop (<Component footer={() => <div />} />), a lot of errors came out from various repositories. Now the detection will exclude props which names are prefixed with render, e.g. <Component renderFooter={() => <div />}. This seems to be quite common pattern.

There's also an option which can be used to disable "component in prop" warnings.

Rule Options

...
"react/no-unstable-nested-components": [
  "off" | "warn" | "error",
  { "allowAsProps": true | false }
]
...

You can allow component creation inside component props by setting allowAsProps option to true. When using this option make sure you are calling the props in the receiving component and not using them as elements.

The following patterns are not considered warnings:

function SomeComponent(props) {
  return <div>{props.footer()}</div>;
}

function Component() {
  return (
    <div>
      <SomeComponent footer={() => <div />} />
    </div>
  );
}

I've tested this plugin with following repositories successfully.

@ljharb can you think of any edge cases where these false positives could easily happen? I've added quite many test cases to the PR now.

AriPerkkio on 16 Aug 2020

It very much will not always be a "stable reference"; a new fragment is created every time it's called, and a new function is passed to the Menu component every time Navigation renders. In other words, you have the most unstable reference you could possibly create.

ljharb on 17 Aug 2020

I think we are not talking about the same thing here, at least in this context. The only thing that matters is that element type is stable.

function Menu(props) {
  const Footer = props.footer;
  return (
    <div>
      {props.children()}
      <Footer />
    </div>
  );
}

function Component() {
  return (
    <Menu
      footer={function SomeFooter() {
        return <>Footer</>;
      }}
    >
      {function Wrapper() {
        return (
          <>
            <div>About</div>
            <div>Contact</div>
          </>
        );
      }}
    </Menu>
  );
}

React reconciler will compare Footer as

function SomeFooter() === function SomeFooter() -> False
Element type has changed, lets create element again. Destroy all children and start over.

And the props.children as

React.Fragment === React.Fragment -> True
Reuse same element, no need to destroy children.
It won't compare Wrapper since given prop is called

It doesn't really matter how many times new fragment is recreated. As long as the element type keeps stable.

current.elementType === element.type

https://github.com/facebook/react/blob/da834083cccb6ef942f701c6b6cecc78213196a8/packages/react-reconciler/src/ReactChildFiber.js#L407

AriPerkkio on 17 Aug 2020

👍1

OK - so by "stable" or "unstable" here, you mean, you want a rule that will warn on any element type (ie, something used as the tag name in jsx, or as the first arg to React.createElement) whose identity can change across renders?

ljharb on 17 Aug 2020

Yes, exactly. I'm not sure how React.createElement could be detected though. I've never been using that API directly.
The current PR will detect these cases from JSX only.

AriPerkkio on 18 Aug 2020

There's a bunch of examples of existing rules that do that; rules must only be jsx-specific whenever possible.

ljharb on 18 Aug 2020

Support for React.createElement cases added. Tested rule against same repositories mentioned earlier.

AriPerkkio on 19 Aug 2020

👍1

In order to identify false positives I've created a tool for linting 100's of repositories easily. After scanning 165 repositories against this rule I've identified one false positive caused by useEffect's clean-up function,.
Also it looks like allowAsProps detection could be improved. Some components created inside props are not marked with correct error message at the moment.

Here are all the results:

AriPerkkio on 11 Oct 2020

👀1 👍1

Another common issue we faced while working on nested components is that hooks of the parent component are sometimes used directly in the nested component. This caused a very hard to identify issue in our component

const App = () => {

  const [count, setCount] = useState(0)

  const myInternalComponent = () => {
    return ( <div>{count}</div> ) // this will cause issues in some scenarios
  }

  return (
    <div>
      {myInternalComponent()}
    </div>
  )
}

DaniAkash on 19 Nov 2020

Thanks for the comment @DaniAkash. These real-world examples are really valuable.

Based on the codesandbox example there is a nested component <EditModal />. This rule will flag that as invalid pattern and mark it as error. Without studying the issue thoroughly, my guess is that the state of <EditModal />'s children is reset during every re-render.

But the pattern of your comment would not be marked as error since element type returned from {myInternalComponent()} is always div. React will not re-construct the component tree when element type is stable. Component below however would be considered as an error:

const App = () => {
  const [count, setCount] = useState(0)

  const MyInternalComponent = () => <div>{count}</div>;

  return (
    <div>
      <MyInternalComponent />
    </div>
  )
}

AriPerkkio on 21 Nov 2020

👍1

Yeah, If the internal component has its own state, it leads to a messy situation

const App = () => {
  const [count, setCount] = useState(0)

  const MyInternalComponent = () => {
    const [internalCount, setInternalCount] = useState(0) // will reset to zero on re-render of App
    return <div>{internalCount}</div>
  };

  return (
    <div>
      <MyInternalComponent />
    </div>
  )
}

DaniAkash on 23 Nov 2020

Obviously it's going to be about impossible to detect cases where a new component type is being passed in as a prop, but this looks like a very useful rule overall.

markerikson on 20 Dec 2020

👍1

The initial post is not up-to-date with current implementation of the PR.

By default this rule will warn if a component is passed as prop and the prop name doesn't start with render, e.g. renderHeader. If component name doesn't start with render it is expected that developer is using the prop as element, e.g. ({ header: Header }) => <Header />.

Here's the current implementation of rule documentation:

By default component creation is allowed inside component props only if prop name starts with render. See allowAsProps option for disabling this limitation completely.

function SomeComponent(props) {
  return <div>{props.renderFooter()}</div>;
}
function Component() {
  return (
    <div>
      <SomeComponent renderFooter={() => <div />} />
    </div>
  );
}

Rule Options

...
"react/no-unstable-nested-components": [
  "off" | "warn" | "error",
  { "allowAsProps": true | false }
]
...

You can allow component creation inside component props by setting allowAsProps option to true. When using this option make sure you are calling the props in the receiving component and not using them as elements.

AriPerkkio on 20 Dec 2020

It may be easier to implement something with fewer false-negatives in some TS-only equivalent of this plugin? Or maybe we could begin making some rules that are only valid in TS environments?