Skip to content

Commit

Permalink
Consider components in jsx as missing dependencies in @typescript-esl…
Browse files Browse the repository at this point in the history
…int/parser@4.x (#19815)

* Run JS tests with TS esling parser

* Add failing test

* fix: Mark JSXIdentifier has missing dependency

* Safe isSameIdentifier
  • Loading branch information
eps1lon authored Sep 11, 2020
1 parent 84558c6 commit 0f70d4d
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function normalizeIndent(strings) {
// }
// ***************************************************

// Tests that are valid/invalid across all parsers
const tests = {
valid: [
{
Expand Down Expand Up @@ -1322,16 +1323,6 @@ const tests = {
}
`,
},
// Ignore Generic Type Variables for arrow functions
{
code: normalizeIndent`
function Example({ prop }) {
const bar = useEffect(<T>(a: T): Hello => {
prop();
}, [prop]);
}
`,
},
// Ignore arguments keyword for arrow functions.
{
code: normalizeIndent`
Expand Down Expand Up @@ -7466,24 +7457,7 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function Foo() {
const foo = ({}: any);
useMemo(() => {
console.log(foo);
}, [foo]);
}
`,
errors: [
{
message:
"The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " +
"Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",
suggestions: undefined,
},
],
},

{
code: normalizeIndent`
function Foo() {
Expand Down Expand Up @@ -7532,6 +7506,43 @@ const tests = {
],
};

// Tests that are only valid/invalid across parsers supporting Flow
const testsFlow = {
valid: [
// Ignore Generic Type Variables for arrow functions
{
code: normalizeIndent`
function Example({ prop }) {
const bar = useEffect(<T>(a: T): Hello => {
prop();
}, [prop]);
}
`,
},
],
invalid: [
{
code: normalizeIndent`
function Foo() {
const foo = ({}: any);
useMemo(() => {
console.log(foo);
}, [foo]);
}
`,
errors: [
{
message:
"The 'foo' object makes the dependencies of useMemo Hook (at line 6) change on every render. " +
"Move it inside the useMemo callback. Alternatively, wrap the initialization of 'foo' in its own useMemo() Hook.",
suggestions: undefined,
},
],
},
],
};

// Tests that are only valid/invalid across parsers supporting TypeScript
const testsTypescript = {
valid: [
{
Expand Down Expand Up @@ -7928,15 +7939,56 @@ const testsTypescript = {
],
};

// Tests that are only valid/invalid for `@typescript-eslint/parser@4.x`
const testsTypescriptEslintParserV4 = {
valid: [],
invalid: [
// TODO: Should also be invalid as part of the JS test suite i.e. be invalid with babel eslint parsers.
// It doesn't use any explicit types but any JS is still valid TS.
{
code: normalizeIndent`
function Foo({ Component }) {
React.useEffect(() => {
console.log(<Component />);
}, []);
};
`,
errors: [
{
message:
"React Hook React.useEffect has a missing dependency: 'Component'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [Component]',
output: normalizeIndent`
function Foo({ Component }) {
React.useEffect(() => {
console.log(<Component />);
}, [Component]);
};
`,
},
],
},
],
},
],
};

// For easier local testing
if (!process.env.CI) {
let only = [];
let skipped = [];
[
...tests.valid,
...tests.invalid,
...testsFlow.valid,
...testsFlow.invalid,
...testsTypescript.valid,
...testsTypescript.invalid,
...testsTypescriptEslintParserV4.valid,
...testsTypescriptEslintParserV4.invalid,
].forEach(t => {
if (t.skip) {
delete t.skip;
Expand All @@ -7958,33 +8010,52 @@ if (!process.env.CI) {
};
tests.valid = tests.valid.filter(predicate);
tests.invalid = tests.invalid.filter(predicate);
testsFlow.valid = testsFlow.valid.filter(predicate);
testsFlow.invalid = testsFlow.invalid.filter(predicate);
testsTypescript.valid = testsTypescript.valid.filter(predicate);
testsTypescript.invalid = testsTypescript.invalid.filter(predicate);
}

describe('react-hooks', () => {
const parserOptions = {
ecmaFeatures: {
jsx: true,
},
ecmaVersion: 6,
sourceType: 'module',
};

const testsBabelEslint = {
valid: [...testsFlow.valid, ...tests.valid],
invalid: [...testsFlow.invalid, ...tests.invalid],
};

new ESLintTester({
parser: require.resolve('babel-eslint'),
parserOptions,
}).run('parser: babel-eslint', ReactHooksESLintRule, tests);
}).run('parser: babel-eslint', ReactHooksESLintRule, testsBabelEslint);

new ESLintTester({
parser: require.resolve('@babel/eslint-parser'),
parserOptions,
}).run('parser: @babel/eslint-parser', ReactHooksESLintRule, tests);
}).run(
'parser: @babel/eslint-parser',
ReactHooksESLintRule,
testsBabelEslint
);

const testsTypescriptEslintParser = {
valid: [...testsTypescript.valid, ...tests.valid],
invalid: [...testsTypescript.invalid, ...tests.invalid],
};

new ESLintTester({
parser: require.resolve('@typescript-eslint/parser-v2'),
parserOptions,
}).run(
'parser: @typescript-eslint/parser@2.x',
ReactHooksESLintRule,
testsTypescript
testsTypescriptEslintParser
);

new ESLintTester({
Expand All @@ -7993,15 +8064,20 @@ describe('react-hooks', () => {
}).run(
'parser: @typescript-eslint/parser@3.x',
ReactHooksESLintRule,
testsTypescript
testsTypescriptEslintParser
);

new ESLintTester({
parser: require.resolve('@typescript-eslint/parser-v4'),
parserOptions,
}).run(
'parser: @typescript-eslint/parser@4.x',
ReactHooksESLintRule,
testsTypescript
);
}).run('parser: @typescript-eslint/parser@4.x', ReactHooksESLintRule, {
valid: [
...testsTypescriptEslintParserV4.valid,
...testsTypescriptEslintParser.valid,
],
invalid: [
...testsTypescriptEslintParserV4.invalid,
...testsTypescriptEslintParser.invalid,
],
});
});
5 changes: 3 additions & 2 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,7 @@ function markNode(node, optionalChains, result) {
* Otherwise throw.
*/
function analyzePropertyChain(node, optionalChains) {
if (node.type === 'Identifier') {
if (node.type === 'Identifier' || node.type === 'JSXIdentifier') {
const result = node.name;
if (optionalChains) {
// Mark as required.
Expand Down Expand Up @@ -1779,7 +1779,8 @@ function isNodeLike(val) {

function isSameIdentifier(a, b) {
return (
a.type === 'Identifier' &&
(a.type === 'Identifier' || a.type === 'JSXIdentifier') &&
a.type === b.type &&
a.name === b.name &&
a.range[0] === b.range[0] &&
a.range[1] === b.range[1]
Expand Down

0 comments on commit 0f70d4d

Please sign in to comment.