Skip to content

Commit 4b02b66

Browse files
authored
[ESLint] Extend isHook to recognize those under PascalCase's namespace (#18722)
* Extend Namespace to PascalCase * Add valid case for jest.useFakeTimer * format * format :( * fix nits
1 parent 30cee2f commit 4b02b66

2 files changed

Lines changed: 58 additions & 47 deletions

File tree

packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js

Lines changed: 56 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ const tests = {
106106
({useHook() { useState(); }});
107107
const {useHook3 = () => { useState(); }} = {};
108108
({useHook = () => { useState(); }} = {});
109+
Namespace.useHook = () => { useState(); };
109110
`,
110111
`
111112
// Valid because hooks can call hooks.
@@ -191,64 +192,22 @@ const tests = {
191192
}
192193
}
193194
`,
194-
`
195-
// Currently valid.
196-
// We *could* make this invalid if we want, but it creates false positives
197-
// (see the FooStore case).
198-
class C {
199-
m() {
200-
This.useHook();
201-
Super.useHook();
202-
}
203-
}
204-
`,
205-
`
206-
// Valid although we *could* consider these invalid.
207-
// But it doesn't bring much benefit since it's an immediate runtime error anyway.
208-
// So might as well allow it.
209-
Hook.use();
210-
Hook._use();
211-
Hook.useState();
212-
Hook._useState();
213-
Hook.use42();
214-
Hook.useHook();
215-
Hook.use_hook();
216-
`,
217195
`
218196
// Valid -- this is a regression test.
219197
jest.useFakeTimers();
220198
beforeEach(() => {
221199
jest.useRealTimers();
222200
})
223201
`,
224-
`
225-
// Valid because that's a false positive we've seen quite a bit.
226-
// This is a regression test.
227-
class Foo extends Component {
228-
render() {
229-
if (cond) {
230-
FooStore.useFeatureFlag();
231-
}
232-
}
233-
}
234-
`,
235202
`
236203
// Valid because they're not matching use[A-Z].
237204
fooState();
238205
use();
239206
_use();
240207
_useState();
241208
use_hook();
242-
`,
243-
`
244-
// This is grey area.
245-
// Currently it's valid (although React.useCallback would fail here).
246-
// We could also get stricter and disallow it, just like we did
247-
// with non-namespace use*() top-level calls.
248-
const History = require('history-2.1.2');
249-
const browserHistory = History.useBasename(History.createHistory)({
250-
basename: '/',
251-
});
209+
// also valid because it's not matching the PascalCase namespace
210+
jest.useFakeTimer()
252211
`,
253212
`
254213
// Regression test for some internal code.
@@ -392,6 +351,59 @@ const tests = {
392351
`,
393352
errors: [conditionalError('useConditionalHook')],
394353
},
354+
{
355+
code: `
356+
Hook.use();
357+
Hook._use();
358+
Hook.useState();
359+
Hook._useState();
360+
Hook.use42();
361+
Hook.useHook();
362+
Hook.use_hook();
363+
`,
364+
errors: [
365+
topLevelError('Hook.useState'),
366+
topLevelError('Hook.use42'),
367+
topLevelError('Hook.useHook'),
368+
],
369+
},
370+
{
371+
code: `
372+
class C {
373+
m() {
374+
This.useHook();
375+
Super.useHook();
376+
}
377+
}
378+
`,
379+
errors: [classError('This.useHook'), classError('Super.useHook')],
380+
},
381+
{
382+
code: `
383+
// This is a false positive (it's valid) that unfortunately
384+
// we cannot avoid. Prefer to rename it to not start with "use"
385+
class Foo extends Component {
386+
render() {
387+
if (cond) {
388+
FooStore.useFeatureFlag();
389+
}
390+
}
391+
}
392+
`,
393+
errors: [classError('FooStore.useFeatureFlag')],
394+
},
395+
{
396+
code: `
397+
// Invalid because it's dangerous and might not warn otherwise.
398+
// This *must* be invalid.
399+
function ComponentWithConditionalHook() {
400+
if (cond) {
401+
Namespace.useConditionalHook();
402+
}
403+
}
404+
`,
405+
errors: [conditionalError('Namespace.useConditionalHook')],
406+
},
395407
{
396408
code: `
397409
// Invalid because it's dangerous and might not warn otherwise.

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,9 @@ function isHook(node) {
3131
!node.computed &&
3232
isHook(node.property)
3333
) {
34-
// Only consider React.useFoo() to be namespace hooks for now to avoid false positives.
35-
// We can expand this check later.
3634
const obj = node.object;
37-
return obj.type === 'Identifier' && obj.name === 'React';
35+
const isPascalCaseNameSpace = /^[A-Z].*/;
36+
return obj.type === 'Identifier' && isPascalCaseNameSpace.test(obj.name);
3837
} else {
3938
return false;
4039
}

0 commit comments

Comments
 (0)