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.

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

[ESLint] Consistently treat optional chaining as regular chaining #19273

Merged
merged 7 commits into from
Jul 7, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,44 @@ const tests = {
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
console.log(props.foo?.bar);
}, [props.foo.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
console.log(props.foo.bar);
}, [props.foo?.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
console.log(props.foo.bar);
console.log(props.foo?.bar);
}, [props.foo?.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
console.log(props.foo.bar);
console.log(props.foo?.bar);
}, [props.foo.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
Expand Down Expand Up @@ -1264,17 +1302,16 @@ const tests = {
errors: [
{
message:
"React Hook useCallback has a missing dependency: 'props.foo?.toString'. " +
"React Hook useCallback has a missing dependency: 'props.foo.toString'. " +
gaearon marked this conversation as resolved.
Show resolved Hide resolved
'Either include it or remove the dependency array.',
suggestions: [
{
desc:
'Update the dependencies array to be: [props.foo?.toString]',
desc: 'Update the dependencies array to be: [props.foo.toString]',
output: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.toString());
}, [props.foo?.toString]);
}, [props.foo.toString]);
}
`,
},
Expand Down Expand Up @@ -1867,18 +1904,18 @@ const tests = {
errors: [
{
message:
"React Hook useEffect has a missing dependency: 'history?.foo'. " +
"React Hook useEffect has a missing dependency: 'history.foo'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [history?.foo]',
desc: 'Update the dependencies array to be: [history.foo]',
output: normalizeIndent`
function MyComponent({ history }) {
useEffect(() => {
return [
history?.foo
];
}, [history?.foo]);
}, [history.foo]);
}
`,
},
Expand Down
47 changes: 16 additions & 31 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -1016,11 +1016,11 @@ export default {
// Is this a variable from top scope?
const topScopeRef = componentScope.set.get(missingDep);
const usedDep = dependencies.get(missingDep);
if (usedDep && usedDep.references[0].resolved !== topScopeRef) {
if (usedDep.references[0].resolved !== topScopeRef) {
return;
}
// Is this a destructured prop?
const def = topScopeRef && topScopeRef.defs[0];
const def = topScopeRef.defs[0];
if (def == null || def.name == null || def.type !== 'Parameter') {
return;
}
Expand Down Expand Up @@ -1062,7 +1062,7 @@ export default {
return;
}
const usedDep = dependencies.get(missingDep);
const references = usedDep ? usedDep.references : [];
const references = usedDep.references;
let id;
let maybeCall;
for (let i = 0; i < references.length; i++) {
Expand Down Expand Up @@ -1238,7 +1238,7 @@ function collectRecommendations({
const keys = path.split('.');
let node = rootNode;
for (const key of keys) {
let child = getChildByKey(node, key);
let child = node.children.get(key);
if (!child) {
child = createDepTree();
node.children.set(key, child);
Expand All @@ -1251,7 +1251,7 @@ function collectRecommendations({
const keys = path.split('.');
let node = rootNode;
for (const key of keys) {
const child = getChildByKey(node, key);
const child = node.children.get(key);
if (!child) {
return;
}
Expand All @@ -1260,21 +1260,6 @@ function collectRecommendations({
}
}

/**
* Match key with optional chaining
* key -> key
* key? -> key
* key -> key?
* Otherwise undefined.
*/
function getChildByKey(node, key) {
return (
node.children.get(key) ||
node.children.get(key.split('?')[0]) ||
node.children.get(key + '?')
);
}

// Now we can learn which dependencies are missing or necessary.
const missingDependencies = new Set();
const satisfyingDependencies = new Set();
Expand All @@ -1287,13 +1272,10 @@ function collectRecommendations({
function scanTreeRecursively(node, missingPaths, satisfyingPaths, keyToPath) {
node.children.forEach((child, key) => {
const path = keyToPath(key);
// For analyzing dependencies, we want the "normalized" path, without any optional chaining ("?.") operator
// foo?.bar -> foo.bar
const normalizedPath = path.replace(/\?$/, '');
if (child.isSatisfiedRecursively) {
if (child.hasRequiredNodesBelow) {
// Remember this dep actually satisfied something.
satisfyingPaths.add(normalizedPath);
satisfyingPaths.add(path);
}
// It doesn't matter if there's something deeper.
// It would be transitively satisfied since we assume immutability.
Expand All @@ -1302,7 +1284,7 @@ function collectRecommendations({
}
if (child.isRequired) {
// Remember that no declared deps satisfied this node.
missingPaths.add(normalizedPath);
missingPaths.add(path);
// If we got here, nothing in its subtree was satisfied.
// No need to search further.
return;
Expand Down Expand Up @@ -1475,20 +1457,23 @@ function getDependency(node) {
* (foo) -> 'foo'
* foo.(bar) -> 'foo.bar'
* foo.bar.(baz) -> 'foo.bar.baz'
* foo?.(bar) -> 'foo?.bar'
* Otherwise throw.
*/
function toPropertyAccessString(node) {
if (node.type === 'Identifier') {
return node.name;
} else if (node.type === 'MemberExpression' && !node.computed) {
} else if (
(node.type === 'MemberExpression' ||
node.type === 'OptionalMemberExpression') &&
!node.computed
) {
const object = toPropertyAccessString(node.object);
const property = toPropertyAccessString(node.property);
// Note: we intentionally omit ? even for optional chaining
// because the returned string represents a path to the node, and
// is used as a key in Maps where being optional doesn't matter.
// The result string is not being interpolated in the code output.
return `${object}.${property}`;
} else if (node.type === 'OptionalMemberExpression' && !node.computed) {
const object = toPropertyAccessString(node.object);
const property = toPropertyAccessString(node.property);
return `${object}?.${property}`;
} else {
throw new Error(`Unsupported node type: ${node.type}`);
}
Expand Down