Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider components in jsx as missing dependencies in @typescript-eslint/parser@4.x #19815

Merged
merged 4 commits into from
Sep 11, 2020
Merged

Consider components in jsx as missing dependencies in @typescript-eslint/parser@4.x #19815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a6e98a
Run JS tests with TS esling parser
eps1lon Sep 11, 2020
032dc52
Add failing test
eps1lon Sep 11, 2020
8367286
fix: Mark JSXIdentifier has missing dependency
eps1lon Sep 11, 2020
553f6cc
Safe isSameIdentifier
eps1lon Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 113 additions & 37 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Open in desktop
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
View file
Open in desktop
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') &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a bit hacky since it would return true when comparing isSameIdentifier(Component, <Component />). Can't imagine a path that would lead to this though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds odd. Can we find a way to tighten it up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check that makes sure a and b are the same kind of identifier. isSameIdentifier-ish is probably more accurate at this point.

a.type === b.type &&
a.name === b.name &&
a.range[0] === b.range[0] &&
a.range[1] === b.range[1]
Expand Down