Revert "Revert "[ESLint] Forbid top-level use*() calls (#16455)"" (#16525)
* Revert "Revert "[ESLint] Forbid top-level use*() calls (#16455)" (#16522)" This reverts commit 507f0fb372705c4c6ac5288db3651019b1d0927b. * Update RulesOfHooks.js
Dan Abramov committed
Aug 21, 2019 at 15:43 UTC
c433fbb593c7f42263c0209d3ff421aea59fca4c
2 files changed
+120
-15
packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
+114
-12
@@ -19,8 +19,17 @@ ESLintTester.setDefaultConfig({
19
},
20
});
21
22
-const eslintTester = new ESLintTester();
23
-eslintTester.run('react-hooks', ReactHooksESLintRule, {
22
+// ***************************************************
23
+// For easier local testing, you can add to any case:
24
+// {
25
+// skip: true,
26
+// --or--
27
+// only: true,
28
+// ...
29
+// }
30
+// ***************************************************
31
+
32
+const tests = {
33
valid: [
34
`
35
// Valid because components can use hooks.
@@ -223,21 +232,20 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
232
(class {i() { useState(); }});
233
`,
234
`
226
- // Currently valid although we *could* consider these invalid.
227
- // It doesn't make a lot of difference because it would crash early.
235
+ // Valid because they're not matching use[A-Z].
236
+ fooState();
237
use();
238
_use();
230
- useState();
239
_useState();
232
- use42();
233
- useHook();
240
use_hook();
235
- React.useState();
241
`,
242
`
238
- // Regression test for the popular "history" library
239
- const {createHistory, useBasename} = require('history-2.1.2');
240
- const browserHistory = useBasename(createHistory)({
243
+ // This is grey area.
244
+ // Currently it's valid (although React.useCallback would fail here).
245
+ // We could also get stricter and disallow it, just like we did
246
+ // with non-namespace use*() top-level calls.
247
+ const History = require('history-2.1.2');
248
+ const browserHistory = History.useBasename(History.createHistory)({
249
basename: '/',
250
});
251
`,
@@ -669,8 +677,63 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
677
conditionalError('useState'),
678
],
679
},
680
+ {
681
+ code: `
682
+ // Invalid because it's dangerous and might not warn otherwise.
683
+ // This *must* be invalid.
684
+ function useHook({ bar }) {
685
+ let foo1 = bar && useState();
686
+ let foo2 = bar || useState();
687
+ let foo3 = bar ?? useState();
688
+ }
689
+ `,
690
+ errors: [
691
+ conditionalError('useState'),
692
+ conditionalError('useState'),
693
+ // TODO: ideally this *should* warn, but ESLint
694
+ // doesn't plan full support for ?? until it advances.
695
+ // conditionalError('useState'),
696
+ ],
697
+ },
698
+ {
699
+ code: `
700
+ // Invalid because it's dangerous.
701
+ // Normally, this would crash, but not if you use inline requires.
702
+ // This *must* be invalid.
703
+ // It's expected to have some false positives, but arguably
704
+ // they are confusing anyway due to the use*() convention
705
+ // already being associated with Hooks.
706
+ useState();
707
+ if (foo) {
708
+ const foo = React.useCallback(() => {});
709
+ }
710
+ useCustomHook();
711
+ `,
712
+ errors: [
713
+ topLevelError('useState'),
714
+ topLevelError('React.useCallback'),
715
+ topLevelError('useCustomHook'),
716
+ ],
717
+ },
718
+ {
719
+ code: `
720
+ // Technically this is a false positive.
721
+ // We *could* make it valid (and it used to be).
722
+ //
723
+ // However, top-level Hook-like calls can be very dangerous
724
+ // in environments with inline requires because they can mask
725
+ // the runtime error by accident.
726
+ // So we prefer to disallow it despite the false positive.
727
+
728
+ const {createHistory, useBasename} = require('history-2.1.2');
729
+ const browserHistory = useBasename(createHistory)({
730
+ basename: '/',
731
+ });
732
+ `,
733
+ errors: [topLevelError('useBasename')],
734
+ },
735
],
673
-});
736
+};
737
738
function conditionalError(hook, hasPreviousFinalizer = false) {
739
return {
@@ -708,3 +771,42 @@ function genericError(hook) {
771
'Hook function.',
772
};
773
}
774
+
775
+function topLevelError(hook) {
776
+ return {
777
+ message:
778
+ `React Hook "${hook}" cannot be called at the top level. React Hooks ` +
779
+ 'must be called in a React function component or a custom React ' +
780
+ 'Hook function.',
781
+ };
782
+}
783
+
784
+// For easier local testing
785
+if (!process.env.CI) {
786
+ let only = [];
787
+ let skipped = [];
788
+ [...tests.valid, ...tests.invalid].forEach(t => {
789
+ if (t.skip) {
790
+ delete t.skip;
791
+ skipped.push(t);
792
+ }
793
+ if (t.only) {
794
+ delete t.only;
795
+ only.push(t);
796
+ }
797
+ });
798
+ const predicate = t => {
799
+ if (only.length > 0) {
800
+ return only.indexOf(t) !== -1;
801
+ }
802
+ if (skipped.length > 0) {
803
+ return skipped.indexOf(t) === -1;
804
+ }
805
+ return true;
806
+ };
807
+ tests.valid = tests.valid.filter(predicate);
808
+ tests.invalid = tests.invalid.filter(predicate);
809
+}
810
+
811
+const eslintTester = new ESLintTester();
812
+eslintTester.run('react-hooks', ReactHooksESLintRule, tests);
packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
+6
-3
@@ -432,9 +432,12 @@ export default {
432
'React Hook function.';
433
context.report({node: hook, message});
434
} else if (codePathNode.type === 'Program') {
435
- // For now, ignore if it's in top level scope.
436
- // We could warn here but there are false positives related
437
- // configuring libraries like `history`.
435
+ // These are dangerous if you have inline requires enabled.
436
+ const message =
437
+ `React Hook "${context.getSource(hook)}" cannot be called ` +
438
+ 'at the top level. React Hooks must be called in a ' +
439
+ 'React function component or a custom React Hook function.';
440
+ context.report({node: hook, message});
441
} else {
442
// Assume in all other cases the user called a hook in some
443
// random function callback. This should usually be true for