Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New] no-unstable-nested-components: Prevent creating unstable components inside components #2750

Merged

Conversation

@AriPerkkio
Copy link
Contributor

@AriPerkkio AriPerkkio commented Aug 9, 2020

Fixes #2749.

This is my first time playing with AST. Feel free to guide me in any way.

@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch from 0e24d1c to a45f3fe Aug 14, 2020
@AriPerkkio AriPerkkio marked this pull request as draft Aug 14, 2020
@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch 2 times, most recently from ca684ef to 35f073e Aug 15, 2020
@AriPerkkio AriPerkkio marked this pull request as ready for review Aug 16, 2020
@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch from 35f073e to f8abd1f Aug 19, 2020
@AriPerkkio
Copy link
Contributor Author

@AriPerkkio AriPerkkio commented Sep 2, 2020

Ready for review. @ljharb could you take a look at this some point?

@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch from f8abd1f to 89134ca Oct 1, 2020
@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch 2 times, most recently from 88d4172 to 79d7658 Oct 15, 2020
Copy link
Collaborator

@ljharb ljharb left a comment

Sorry for the long delay. Overall, this looks great!

lib/rules/no-unstable-nested-components.js Outdated Show resolved Hide resolved
const ERROR_MESSAGE_WITHOUT_NAME = 'Declare this component outside parent component or memoize it.';
const COMPONENT_AS_PROPS_INFO = ' If you want to allow component creation in props, set allowAsProps option to true.';
const RENDER_REGEXP = /^render/;
const HOOK_REGEXP = /^use[A-Z0-9].*$/;

This comment has been minimized.

@ljharb

ljharb Nov 6, 2020
Collaborator

it'd be ideal to avoid hardcoding this, to avoid deviating from eslint-plugin-react-with-hooks. is there any way we could share hook detection code with that plugin?

This comment has been minimized.

@AriPerkkio

AriPerkkio Nov 8, 2020
Author Contributor

At the moment, no. Currently they are not exporting the hook detection methods - I just decided to copy-paste the implementation. If they did export this, we could rely on that via dependencies/peerDependencies.

It's a shame hook related rules live in a separate package.

@ljharb ljharb added the new rule label Nov 6, 2020
@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch 2 times, most recently from 3adeaa0 to 5906897 Nov 8, 2020
@AriPerkkio
Copy link
Contributor Author

@AriPerkkio AriPerkkio commented Dec 20, 2020

As the component detection in various patterns is tricky I think this rule should be run with eslint-remote-tester against 100-500 repositories before merging, once the implementation is reviewed and accepted. I could upload the results in another branch and check whether any false positives have been found.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Dec 30, 2020

Please mark this PR as ready for review when you're confident in the results, and we can land it.

@ljharb ljharb marked this pull request as draft Dec 30, 2020
AriPerkkio added a commit to AriPerkkio/eslint-plugin-react that referenced this pull request Dec 30, 2020
AriPerkkio added a commit to AriPerkkio/eslint-plugin-react that referenced this pull request Jan 1, 2021
@AriPerkkio AriPerkkio force-pushed the AriPerkkio:react/no-unstable-nested-components branch from 5906897 to a46394b Jan 1, 2021
@AriPerkkio AriPerkkio marked this pull request as ready for review Jan 1, 2021
@AriPerkkio
Copy link
Contributor Author

@AriPerkkio AriPerkkio commented Jan 1, 2021

Final testing is now complete. I tested this against ~550 repositories. Fixed false positives of three rare minor cases. I'm confident to ship this now @ljharb.

There are two false positive cases remaining which I'm unable to fix for now. These are caused by component.get(node) falsely marking them as components. These cases are found from test cases as placeholders:

    /* TODO These minor cases are currently falsely marked due to component detection
    {
      code: `
      function ParentComponent() {
        const _renderHeader = () => <div />;
        return <div>{_renderHeader()}</div>;
      }
      `
    },
    {
      // https://github.com/emotion-js/emotion/blob/a89d4257b0246a1640a99f77838e5edad4ec4386/packages/jest/test/react-enzyme.test.js#L26-L28
      code: `
      const testCases = {
        basic: {
          render() {
            const Component = () => <div />;
            return <div />;
          }
        }
      }
      `
    },
    */
@ljharb
ljharb approved these changes Mar 23, 2021
lib/rules/no-unstable-nested-components.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the AriPerkkio:react/no-unstable-nested-components branch from a46394b to 7b35ee7 Mar 23, 2021
@ljharb ljharb merged commit 7b35ee7 into yannickcr:master Mar 23, 2021
43 checks passed
43 checks passed
@github-actions
Automatic Rebase
Details
@github-actions
Require “Allow Edits”
Details
@github-actions
matrix
Details
@github-actions
pretest
Details
@github-actions
readme
Details
@github-actions
latest majors (15, 7)
Details
@github-actions
posttest
Details
@github-actions
latest majors (15, 6)
Details
@github-actions
latest majors (15, 5)
Details
@github-actions
latest majors (15, 4)
Details
@github-actions
latest majors (14, 7)
Details
@github-actions
latest majors (14, 6)
Details
@github-actions
latest majors (14, 5)
Details
@github-actions
latest majors (14, 4)
Details
@github-actions
latest majors (13, 7)
Details
@github-actions
latest majors (13, 6)
Details
@github-actions
latest majors (13, 5)
Details
@github-actions
latest majors (13, 4)
Details
@github-actions
latest majors (12, 7)
Details
@github-actions
latest majors (12, 6)
Details
@github-actions
latest majors (12, 5)
Details
@github-actions
latest majors (12, 4)
Details
@github-actions
latest majors (11, 7)
Details
@github-actions
latest majors (11, 6)
Details
@github-actions
latest majors (11, 5)
Details
@github-actions
latest majors (11, 4)
Details
@github-actions
latest majors (10, 7)
Details
@github-actions
latest majors (10, 6)
Details
@github-actions
latest majors (10, 5)
Details
@github-actions
latest majors (10, 4)
Details
@github-actions
latest majors (9, 6)
Details
@github-actions
latest majors (9, 5)
Details
@github-actions
latest majors (9, 4)
Details
@github-actions
latest majors (8, 6)
Details
@github-actions
latest majors (8, 5)
Details
@github-actions
latest majors (8, 4)
Details
@github-actions
latest majors (7, 5)
Details
@github-actions
latest majors (7, 4)
Details
@github-actions
latest majors (6, 5)
Details
@github-actions
latest majors (6, 4)
Details
@github-actions
latest majors (5, 4)
Details
@github-actions
latest majors (4, 4)
Details
@github-actions
node 4+
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants