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

ESLint Plugin: Exempt React hooks from no-unused-vars-before-return #16737

Merged
merged 3 commits into from Jul 25, 2019

Conversation

@aduth
Copy link
Member

commented Jul 24, 2019

This pull request seeks to improve interoperability between our custom @wordpress/no-unused-vars-before-return and React Hooks. Since hooks must appear in a deterministic order, they occasionally must break this rule.

The implementation here adds a new excludePattern to the rule, which is then used by the default React configuration to exclude function calls prefixed with the use convention.

Testing Instructions:

Verify there are no lint errors:

npm run lint-js

Verify that implementing a variable assignment where the variable is used after a return path does not trigger the @wordpress/no-unused-vars-before-return rule.

@@ -12,6 +12,9 @@ module.exports = {
'react-hooks',
],
rules: {
'@wordpress/no-unused-vars-before-return': [ 'error', {
excludePattern: '^use',

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

Does this pattern work for all kind of things or only function calls? Maybe the naming should indicate that it's about function calls?

This comment has been minimized.

Copy link
@aduth

aduth Jul 25, 2019

Author Member

Does this pattern work for all kind of things or only function calls? Maybe the naming should indicate that it's about function calls?

Today, the rule is only concerned with function calls, as a rough trigger for what should be considered expensive:

// Target function calls as "expensive".
def.node.init.type === 'CallExpression' &&

I suppose this doesn't always need to be the case, and in the future we could be more inclusive on what we consider to be "expensive". We can be more open in this module to future breaking changes, but I think it's a sensible change to make even now, in line with what the rule is intended to enforce.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 25, 2019

Contributor

Yes, not a big concern.

This comment has been minimized.

Copy link
@aduth

aduth Jul 25, 2019

Author Member

Hm, let's leave it as is for now. For the moment, the description of the rule is pretty explicit about this applying to functions:

To avoid wastefully computing the result of a function call when assigning a variable value

...and I'm not inclined to think this will change anytime soon.

@aduth aduth merged commit 9fcccb3 into master Jul 25, 2019

1 of 5 checks passed

Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the add/eslint-no-unused-vars-exception branch Jul 25, 2019

@github-actions github-actions bot added this to the Gutenberg 6.2 milestone Jul 25, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

ESLint Plugin: Exempt React hooks from no-unused-vars-before-return (W…
…ordPress#16737)

* ESLint Plugin: No Unused Vars Before Return: Add option excludePattern

* ESLint Plugin: Exempt hooks from recommended react configuration

* Components: Remove unneccessary ESLint disabling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.