Skip to content

Commit

Permalink
[eslint-plugin-react-hooks] Disallow hooks in class components (#18341)
Browse files Browse the repository at this point in the history
Per discussion at Facebook, we think hooks have reached a tipping point where it is more valuable to lint against potential hooks in classes than to worry about false positives.

Test plan:
```
# run from repo root
yarn test --watch RuleOfHooks
```
  • Loading branch information
ianobermiller committed Mar 18, 2020
1 parent 8206b4b commit d3368be
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -232,45 +232,6 @@ const tests = {
}
}
`,
`
// Currently valid because we found this to be a common pattern
// for feature flag checks in existing components.
// We *could* make it invalid but that produces quite a few false positives.
// Why does it make sense to ignore it? Firstly, because using
// hooks in a class would cause a runtime error anyway.
// But why don't we care about the same kind of false positive in a functional
// component? Because even if it was a false positive, it would be confusing
// anyway. So it might make sense to rename a feature flag check in that case.
class ClassComponentWithFeatureFlag extends React.Component {
render() {
if (foo) {
useFeatureFlag();
}
}
}
`,
`
// Currently valid because we don't check for hooks in classes.
// See ClassComponentWithFeatureFlag for rationale.
// We *could* make it invalid if we don't regress that false positive.
class ClassComponentWithHook extends React.Component {
render() {
React.useState();
}
}
`,
`
// Currently valid.
// These are variations capturing the current heuristic--
// we only allow hooks in PascalCase, useFoo functions,
// or classes (due to common false positives and because they error anyway).
// We *could* make some of these invalid.
// They probably don't matter much.
(class {useHook = () => { useState(); }});
(class {useHook() { useState(); }});
(class {h = () => { useState(); }});
(class {i() { useState(); }});
`,
`
// Valid because they're not matching use[A-Z].
fooState();
Expand Down Expand Up @@ -870,6 +831,52 @@ const tests = {
`,
errors: [topLevelError('useBasename')],
},
{
code: `
class ClassComponentWithFeatureFlag extends React.Component {
render() {
if (foo) {
useFeatureFlag();
}
}
}
`,
errors: [classError('useFeatureFlag')],
},
{
code: `
class ClassComponentWithHook extends React.Component {
render() {
React.useState();
}
}
`,
errors: [classError('React.useState')],
},
{
code: `
(class {useHook = () => { useState(); }});
`,
errors: [classError('useState')],
},
{
code: `
(class {useHook() { useState(); }});
`,
errors: [classError('useState')],
},
{
code: `
(class {h = () => { useState(); }});
`,
errors: [classError('useState')],
},
{
code: `
(class {i() { useState(); }});
`,
errors: [classError('useState')],
},
],
};

Expand Down Expand Up @@ -919,6 +926,15 @@ function topLevelError(hook) {
};
}

function classError(hook) {
return {
message:
`React Hook "${hook}" cannot be called in a class component. React Hooks ` +
'must be called in a React function component or a custom React ' +
'Hook function.',
};
}

// For easier local testing
if (!process.env.CI) {
let only = [];
Expand Down
11 changes: 6 additions & 5 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,11 +461,12 @@ export default {
codePathNode.parent.type === 'ClassProperty') &&
codePathNode.parent.value === codePathNode
) {
// Ignore class methods for now because they produce too many
// false positives due to feature flag checks. We're less
// sensitive to them in classes because hooks would produce
// runtime errors in classes anyway, and because a use*()
// call in a class, if it works, is unambiguously *not* a hook.
// Custom message for hooks inside a class
const message =
`React Hook "${context.getSource(hook)}" cannot be called ` +
'in a class component. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
} else if (codePathFunctionName) {
// Custom message if we found an invalid function name.
const message =
Expand Down

0 comments on commit d3368be

Please sign in to comment.