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

Fixes #2749.

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

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

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

@AriPerkkio AriPerkkio force-pushed the react/no-unstable-nested-components branch from f8abd1f to 89134ca Compare October 1, 2020 17:17
@AriPerkkio AriPerkkio force-pushed the react/no-unstable-nested-components branch 2 times, most recently from 88d4172 to 79d7658 Compare October 17, 2020 07:06
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].*$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 react/no-unstable-nested-components branch 2 times, most recently from 3adeaa0 to 5906897 Compare November 14, 2020 11:05
@AriPerkkio
Copy link
Contributor Author

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
Member

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 December 30, 2020 04:17
@AriPerkkio AriPerkkio force-pushed the react/no-unstable-nested-components branch from 5906897 to a46394b Compare January 1, 2021 11:02
@AriPerkkio AriPerkkio marked this pull request as ready for review January 1, 2021 11:06
@AriPerkkio
Copy link
Contributor Author

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 />;
          }
        }
      }
      `
    },
    */

lib/rules/no-unstable-nested-components.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Rule proposal: react/no-unstable-nested-components
2 participants