Skip to content

Commit

Permalink
[ESLint] Extend isHook to recognize those under PascalCase's namespace (
Browse files Browse the repository at this point in the history
#18722)

* Extend Namespace to PascalCase

* Add valid case for jest.useFakeTimer

* format

* format :(

* fix nits
  • Loading branch information
cyan33 committed Apr 24, 2020
1 parent 30cee2f commit 4b02b66
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 47 deletions.
Expand Up @@ -106,6 +106,7 @@ const tests = {
({useHook() { useState(); }});
const {useHook3 = () => { useState(); }} = {};
({useHook = () => { useState(); }} = {});
Namespace.useHook = () => { useState(); };
`,
`
// Valid because hooks can call hooks.
Expand Down Expand Up @@ -191,64 +192,22 @@ const tests = {
}
}
`,
`
// Currently valid.
// We *could* make this invalid if we want, but it creates false positives
// (see the FooStore case).
class C {
m() {
This.useHook();
Super.useHook();
}
}
`,
`
// Valid although we *could* consider these invalid.
// But it doesn't bring much benefit since it's an immediate runtime error anyway.
// So might as well allow it.
Hook.use();
Hook._use();
Hook.useState();
Hook._useState();
Hook.use42();
Hook.useHook();
Hook.use_hook();
`,
`
// Valid -- this is a regression test.
jest.useFakeTimers();
beforeEach(() => {
jest.useRealTimers();
})
`,
`
// Valid because that's a false positive we've seen quite a bit.
// This is a regression test.
class Foo extends Component {
render() {
if (cond) {
FooStore.useFeatureFlag();
}
}
}
`,
`
// Valid because they're not matching use[A-Z].
fooState();
use();
_use();
_useState();
use_hook();
`,
`
// This is grey area.
// Currently it's valid (although React.useCallback would fail here).
// We could also get stricter and disallow it, just like we did
// with non-namespace use*() top-level calls.
const History = require('history-2.1.2');
const browserHistory = History.useBasename(History.createHistory)({
basename: '/',
});
// also valid because it's not matching the PascalCase namespace
jest.useFakeTimer()
`,
`
// Regression test for some internal code.
Expand Down Expand Up @@ -392,6 +351,59 @@ const tests = {
`,
errors: [conditionalError('useConditionalHook')],
},
{
code: `
Hook.use();
Hook._use();
Hook.useState();
Hook._useState();
Hook.use42();
Hook.useHook();
Hook.use_hook();
`,
errors: [
topLevelError('Hook.useState'),
topLevelError('Hook.use42'),
topLevelError('Hook.useHook'),
],
},
{
code: `
class C {
m() {
This.useHook();
Super.useHook();
}
}
`,
errors: [classError('This.useHook'), classError('Super.useHook')],
},
{
code: `
// This is a false positive (it's valid) that unfortunately
// we cannot avoid. Prefer to rename it to not start with "use"
class Foo extends Component {
render() {
if (cond) {
FooStore.useFeatureFlag();
}
}
}
`,
errors: [classError('FooStore.useFeatureFlag')],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
// This *must* be invalid.
function ComponentWithConditionalHook() {
if (cond) {
Namespace.useConditionalHook();
}
}
`,
errors: [conditionalError('Namespace.useConditionalHook')],
},
{
code: `
// Invalid because it's dangerous and might not warn otherwise.
Expand Down
5 changes: 2 additions & 3 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
Expand Up @@ -31,10 +31,9 @@ function isHook(node) {
!node.computed &&
isHook(node.property)
) {
// Only consider React.useFoo() to be namespace hooks for now to avoid false positives.
// We can expand this check later.
const obj = node.object;
return obj.type === 'Identifier' && obj.name === 'React';
const isPascalCaseNameSpace = /^[A-Z].*/;
return obj.type === 'Identifier' && isPascalCaseNameSpace.test(obj.name);
} else {
return false;
}
Expand Down

0 comments on commit 4b02b66

Please sign in to comment.