Add no-nested-components ESLint rule #25360
Conversation
There was a problem hiding this comment.
I ran this through 2000 repositories. No crashes but some false positives. Most are from uppercase functions which do not return JSX - the eslint-plugin-react/no-unstable-nested-components rule especially checks whether function is returning JSX or React.createElement. Maybe this one should do too.
Copy-paste ready test cases:
{
code: normalizeIndent`
function ParentComponent() {
function NotComponent() {
console.log('Not returning JSX');
}
return <button onClick={NotComponent} />;
}
`,
},
{
code: normalizeIndent`
function ParentComponent() {
const inputKeyDownHandlers = useMemo(() => ({
ArrowDown(event) {
event.preventDefault();
// Do something
},
ArrowUp(event) {
event.preventDefault();
// Do something
},
}));
return <ComplexComponent handlers={inputKeyDownHandlers} />;
}
`,
},
{
code: normalizeIndent`
const MyComponent = React.memo(
React.forwardRef((props, ref) => {
return <div />
})
)
`,
},Uppercase component not returning JSX
Rule: react-hooks/no-nested-components
- Message:
Component "CleanupInput" is declared during render. You should move this declaration outside of render to ensure this component's state is persisted across re-renders of its parent. If this component is memoized, you should still refactor the component to be able to move it outside of render. If you want to reset its state use a key instead (see https://reactjs.org/docs/lists-and-keys.html#keys). - Path:
axmz/react-searchbox-awesome/src/components/Search.jsx - Link
82 |
83 | // Cleanup input
> 84 | function CleanupInput() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
> 85 | inputRef.current.value = ""
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 86 | inputRef.current.dispatchEvent(inputEvent)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 87 | inputRef.current.focus();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 88 | }
| ^^^^
89 |
90 | // Click handler
91 | const clickHandler = (e) => {Memo + forwardRef
Rule: react-hooks/no-nested-components
- Message:
Component "Unknown" is declared during render. You should move this declaration outside of render to ensure this component's state is persisted across re-renders of its parent. If this component is memoized, you should still refactor the component to be able to move it outside of render. If you want to reset its state use a key instead (see https://reactjs.org/docs/lists-and-keys.html#keys). - Path:
alloc/wana/packages/babel-plugin-add-react-displayname/spec/fixtures/multipleWraps.tsx - Link
1 | export const Test1 = React.memo(
> 2 | React.forwardRef((props, ref) => {
| ^^^^^^^^^^^^^^^^^
> 3 | return <div />
| ^^^^^^^^^^^^^^^^^^
> 4 | })
| ^^^^
5 | )
6 |
7 | const Test2 = React.memo(Hook-like, not returninig JSX
Rule: react-hooks/no-nested-components
- Message:
Component "useBlock" is declared during render. You should move this declaration outside of render to ensure this component's state is persisted across re-renders of its parent. If this component is memoized, you should still refactor the component to be able to move it outside of render. If you want to reset its state use a key instead (see https://reactjs.org/docs/lists-and-keys.html#keys). - Path:
BuilderIO/jsx-lite/packages/core/src/__tests__/data/blocks/builder-render-block.raw.tsx - Link
46 | return getBlockProperties(state.useBlock);
47 | },
> 48 | get useBlock() {
| ^^^^
> 49 | return getProcessedBlock({
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 50 | block: props.block,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 51 | state: builderContext.state,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 52 | context: builderContext.context,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 53 | });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 54 | },
| ^^^^^^
55 | get actions() {
56 | return getBlockActions({
57 | block: state.useBlock,Memoized objcet with uppercase functions
Rule: react-hooks/no-nested-components
- Message:
Component "ArrowDown" is declared during render. You should move this declaration outside of render to ensure this component's state is persisted across re-renders of its parent. If this component is memoized, you should still refactor the component to be able to move it outside of render. If you want to reset its state use a key instead (see https://reactjs.org/docs/lists-and-keys.html#keys). - Path:
downshift-js/downshift/src/hooks/useCombobox/index.js - Link
153 | const inputKeyDownHandlers = useMemo(
154 | () => ({
> 155 | ArrowDown(event) {
| ^^^^^^^^^
> 156 | event.preventDefault()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 157 | dispatch({
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 158 | type: stateChangeTypes.InputKeyDownArrowDown,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 159 | shiftKey: event.shiftKey,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 160 | getItemNodeFromIndex,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 161 | })
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 162 | },
| ^^^^^^^^
163 | ArrowUp(event) {
164 | event.preventDefault()
165 | dispatch({
How did you do that?
In these false-positives, what would the existing react-hooks rules say? The idea was that it should flag everything that could also contain a hooks call. So "false-positives" of this rule would be false-negatives of the other rules i.e. it's not considered a false-positive since we established that uppercase functions are components. In those cases the author should just rename
The memo+forwardRef is definitely something we should not flag 👍🏻
That case needs a better message but is not a false-positive. The function is considered a hook and can thus be called during render. |
Existing
I agree this should be the recommended solution but not sure how this could be informed to users. Now this rule would confuse users by calling these functions components. Instead the rule should detect that these are not returning JSX, and recommend users to rename the function into lowercase.
Started your react fork in Github Codespaces and installed eslint-remote-tester.config.jsconst eslintRemoteTesterRepositories = require('eslint-remote-tester-repositories');
module.exports = {
repositories: eslintRemoteTesterRepositories
.getRepositories({ randomize: false })
.slice(0, 2000),
pathIgnorePattern: eslintRemoteTesterRepositories.getPathIgnorePattern(),
extensions: ['.js', '.jsx', '.ts', '.tsx'],
rulesUnderTesting: ['react-hooks/no-nested-components'],
cache: false,
eslintrc: {
root: true,
env: {
es6: true,
},
parser: '@typescript-eslint/parser',
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
ecmaFeatures: {
jsx: true,
},
},
plugins: ['react-hooks'],
rules: {
'react-hooks/no-nested-components': ['error'],
},
},
}; |
Which is the problem. React thinks these are function components and allows calling hooks. And if it's a component, it should not allow nested declarations. Either we start flagging hook calls in these cases or we continue flagging nested component declarations. But having an inconsistent detection is the worst scenario since it requires knowledge how individual rules detect components. When you should just need to know what the plugin (or rather React itself) considers a function component.
This has been the narrative since rules-of-hooks was released (see https://reactjs.org/docs/hooks-faq.html#what-exactly-do-the-lint-rules-enforce). We can amend the error message to explain why this is considered a component and how to fix it by renaming.
A component does not need to return JSX. It can return primitives as well. I would not change this detection mechanism in this PR. It should be consistent with the existing rules-of-hooks. If there are real-world scenarios where renaming does not make sense, we can follow-up later. |
eps1lon
force-pushed
the
feat/no-nested-components
branch
from
a97f521
to
e002fc7
Compare
eps1lon
requested review from
AriPerkkio,
sophiebits,
poteto and
gaearon
and removed request for
AriPerkkio
There was a problem hiding this comment.
React thinks these are function components and allows calling hooks.
[...]
This has been the narrative since rules-of-hooks was released (see https://reactjs.org/docs/hooks-faq.html#what-exactly-do-the-lint-rules-enforce).
Oh I see, this is really good. Having a clear convention of what is considered as a component makes writing rules a lot easier.
I did some re-testing with the latest changes and did not see any other problems - a lot of valid ESLint reports did pop up.
But it's also worth to mention, that if this rule is used by default in ESLint configuration, it might start to flag a lot of issues in codebases at first. There are libraries which promote such pattern, e.g. react-table, https://gist.github.com/AriPerkkio/22bb98633d367156d090dafe376444ac. In all of these cases the components could be moved out of render block though.
| function Component() { | ||
| // nested component declaration | ||
| function UnstableNestedComponent() { | ||
| return <div />; | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| <UnstableNestedComponent /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should all mentions of UnstableNestedComponent be simply just NestedComponent? There is no such thing as stable nested component. When I came up with this term, I falsely thought useCallback or useMemo would "stabilize" it.
| function Component() { | |
| // nested component declaration | |
| function UnstableNestedComponent() { | |
| return <div />; | |
| } | |
| return ( | |
| <div> | |
| <UnstableNestedComponent /> | |
| </div> | |
| ); | |
| } | |
| function Component() { | |
| // Nested component declaration | |
| function NestedComponent() { | |
| return <div />; | |
| } | |
| return ( | |
| <div> | |
| <NestedComponent /> | |
| </div> | |
| ); | |
| } |
There was a problem hiding this comment.
Good point. Just copied these tests but I should make a pass regarding naming
…s behavior closer to facebook/react#25360 (#255)
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
github-actions
bot
added
the
Resolution: Stale
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
Adds a new
no-nested-componentsrule toeslint-plugin-react-hooksthat triggers on nested component declarations e.g.These declarations are problematic since the nested components will not persist state across re-renders of the parent component.
The tests are copied from
react/no-nested-unstable-componentsbut re-ordered since we currently apply the same logic as rules-of-hooks: If it looks like a component and is inside a component or hooks it cannot be nested inside another component.Unlike
react/no-nested-unstable-componentswe currently don't detect nested component definitions in class components nor do we detect nested class component definitions. Both of these scenarios would be nice to cover as well though!Open questions:
react/no-unstable-nested-componentsandreact-hooks/no-nested-components?How did you test this change?
Why not recommend
react/no-nested-unstable-componentsno-unstable-nested-componentsis not actually what we want. It's just about nesting components. Memoizing them will still cause breakage since their state won't be re-useable (remember that useMemo/useCallback are perf optimizations not semantic guarantees unlikekeyand component types)Not detected in proposed rule but detected in
react/no-unstable-nested-componentsA.
<Component footer={() => <div />} />This is probably correct to no longer detect since
const footer = () => <div />} />would also not be considered a component. However,<Component Footer={() => <div />} />would also not be detected. This would probably be nice to get into Rules of Hooks regardless.Detected in proposed rule but not detected in
react/no-unstable-nested-componentsA.
Intended. Hooks are called during render which means the component declaration happens during render.
B.
Intended. Rules of Hooks would allow calling a Hook in
ConditionalWrapperso we should consider it a nested declaration.C.
Intended. Even though flagging this particular example seems silly, Rules of Hooks would allow calling a Hook in
ActionIconso we should consider it a nested declaration.