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

* Revert "Fix ExhaustiveDeps ESLint rule throwing with optional chaining (#19260)"

This reverts commit 0f84b0f02b.

* Re-add a test from #19260

* Remove all code for optional chaining support

* Consistently treat optional chaining as regular chaining

This is not ideal because our suggestions use normal chaining. But it gets rid of all current edge cases.

* Add more tests

* More consistency in treating normal and optional expressions

* Add regression tests for every occurrence of Optional*
This commit is contained in:
Dan Abramov 2020-07-07 17:38:44 +01:00 committed by GitHub
parent 98390f11f6
commit 7ca1d861e8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 396 additions and 43 deletions

View File

@ -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) {
@ -307,6 +345,42 @@ const tests = {
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo.bar?.toString());
}, [props.foo.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar?.toString());
}, [props.foo.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo.bar.toString());
}, [props?.foo?.bar]);
}
`,
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar?.baz);
}, [props?.foo.bar?.baz]);
}
`,
},
{
code: normalizeIndent`
function MyComponent() {
@ -692,6 +766,24 @@ const tests = {
}
`,
},
{
// Valid because we assign ref.current
// ourselves. Therefore it's likely not
// a ref managed by React.
code: normalizeIndent`
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
myRef.current = {};
return () => {
console.log(myRef?.current?.toString())
};
}, []);
return <div />;
}
`,
},
{
// Valid because we assign ref.current
// ourselves. Therefore it's likely not
@ -1264,17 +1356,103 @@ const tests = {
errors: [
{
message:
"React Hook useCallback has a missing dependency: 'props.foo?.toString'. " +
"React Hook useCallback has a missing dependency: 'props.foo'. " +
'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]',
output: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.toString());
}, [props.foo?.toString]);
}, [props.foo]);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar.baz);
}, []);
}
`,
errors: [
{
message:
// TODO: Ideally this would suggest props.foo?.bar.baz instead.
"React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo.bar.baz]',
output: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar.baz);
}, [props.foo.bar.baz]);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar?.baz);
}, []);
}
`,
errors: [
{
message:
// TODO: Ideally this would suggest props.foo?.bar?.baz instead.
"React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo.bar.baz]',
output: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar?.baz);
}, [props.foo.bar.baz]);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar.toString());
}, []);
}
`,
errors: [
{
message:
// TODO: Ideally this would suggest props.foo?.bar instead.
"React Hook useCallback has a missing dependency: 'props.foo.bar'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo.bar]',
output: normalizeIndent`
function MyComponent(props) {
useCallback(() => {
console.log(props.foo?.bar.toString());
}, [props.foo.bar]);
}
`,
},
@ -1867,18 +2045,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]);
}
`,
},
@ -3479,6 +3657,47 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1?.current?.focus();
console.log(ref2?.current?.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [ref1?.current, ref2?.current, props.someOtherRefs, props.color]);
}
`,
errors: [
{
message:
"React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " +
'Either exclude them or remove the dependency array. ' +
"Mutable values like 'ref1.current' aren't valid dependencies " +
"because mutating them doesn't re-render the component.",
suggestions: [
{
desc:
'Update the dependencies array to be: [props.someOtherRefs, props.color]',
output: normalizeIndent`
function MyComponent(props) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1?.current?.focus();
console.log(ref2?.current?.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [props.someOtherRefs, props.color]);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent() {
@ -3683,6 +3902,42 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
if (props?.onChange) {
props?.onChange();
}
}, []);
}
`,
errors: [
{
message:
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array. ' +
`However, 'props' will change when *any* prop changes, so the ` +
`preferred fix is to destructure the 'props' object outside ` +
`of the useEffect call and refer to those specific ` +
`props inside useEffect.`,
suggestions: [
{
desc: 'Update the dependencies array to be: [props]',
output: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
if (props?.onChange) {
props?.onChange();
}
}, [props]);
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
@ -4074,6 +4329,29 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function MyComponent() {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
myRef?.current?.addEventListener('mousemove', handleMove);
return () => myRef?.current?.removeEventListener('mousemove', handleMove);
}, []);
return <div ref={myRef} />;
}
`,
errors: [
{
message:
`The ref value 'myRef.current' will likely have changed by the time ` +
`this effect cleanup function runs. If this ref points to a node ` +
`rendered by React, copy 'myRef.current' to a variable inside the effect, ` +
`and use that variable in the cleanup function.`,
suggestions: undefined,
},
],
},
{
code: normalizeIndent`
function MyComponent() {
@ -4448,6 +4726,51 @@ const tests = {
},
],
},
{
code: normalizeIndent`
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
const fn = useCallback(() => {
// nothing
}, [MutableStore?.hello?.world, props.foo, x, y, z, global?.stuff]);
}
}
`,
errors: [
{
message:
'React Hook useCallback has unnecessary dependencies: ' +
"'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " +
'Either exclude them or remove the dependency array. ' +
"Outer scope values like 'MutableStore.hello.world' aren't valid dependencies " +
"because mutating them doesn't re-render the component.",
suggestions: [
{
desc: 'Update the dependencies array to be: []',
output: normalizeIndent`
import MutableStore from 'store';
let z = {};
function MyComponent(props) {
let x = props.foo;
{
let y = props.bar;
const fn = useCallback(() => {
// nothing
}, []);
}
}
`,
},
],
},
],
},
{
// Every almost-static function is tainted by a dynamic value.
code: normalizeIndent`
@ -6008,6 +6331,40 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function Podcasts({ fetchPodcasts, id }) {
let [podcasts, setPodcasts] = useState(null);
useEffect(() => {
console.log(fetchPodcasts);
fetchPodcasts?.(id).then(setPodcasts);
}, [id]);
}
`,
errors: [
{
message:
`React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
`Either include it or remove the dependency array. ` +
`If 'fetchPodcasts' changes too often, ` +
`find the parent component that defines it and wrap that definition in useCallback.`,
suggestions: [
{
desc: 'Update the dependencies array to be: [fetchPodcasts, id]',
output: normalizeIndent`
function Podcasts({ fetchPodcasts, id }) {
let [podcasts, setPodcasts] = useState(null);
useEffect(() => {
console.log(fetchPodcasts);
fetchPodcasts?.(id).then(setPodcasts);
}, [fetchPodcasts, id]);
}
`,
},
],
},
],
},
{
// The mistake here is that it was moved inside the effect
// so it can't be referenced in the deps array.

View File

@ -525,7 +525,8 @@ export default {
isEffect &&
// ... and this look like accessing .current...
dependencyNode.type === 'Identifier' &&
dependencyNode.parent.type === 'MemberExpression' &&
(dependencyNode.parent.type === 'MemberExpression' ||
dependencyNode.parent.type === 'OptionalMemberExpression') &&
!dependencyNode.parent.computed &&
dependencyNode.parent.property.type === 'Identifier' &&
dependencyNode.parent.property.name === 'current' &&
@ -587,6 +588,7 @@ export default {
if (
parent != null &&
// ref.current
// Note: no need to handle OptionalMemberExpression because it can't be LHS.
parent.type === 'MemberExpression' &&
!parent.computed &&
parent.property.type === 'Identifier' &&
@ -790,7 +792,10 @@ export default {
}
let maybeID = declaredDependencyNode;
while (maybeID.type === 'MemberExpression') {
while (
maybeID.type === 'MemberExpression' ||
maybeID.type === 'OptionalMemberExpression'
) {
maybeID = maybeID.object;
}
const isDeclaredInComponent = !componentScope.through.some(
@ -991,7 +996,10 @@ export default {
isPropsOnlyUsedInMembers = false;
break;
}
if (parent.type !== 'MemberExpression') {
if (
parent.type !== 'MemberExpression' &&
parent.type !== 'OptionalMemberExpression'
) {
isPropsOnlyUsedInMembers = false;
break;
}
@ -1016,11 +1024,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;
}
@ -1032,7 +1040,8 @@ export default {
if (
id != null &&
id.parent != null &&
id.parent.type === 'CallExpression' &&
(id.parent.type === 'CallExpression' ||
id.parent.type === 'OptionalCallExpression') &&
id.parent.callee === id
) {
isFunctionCall = true;
@ -1062,7 +1071,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++) {
@ -1238,7 +1247,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);
@ -1251,7 +1260,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;
}
@ -1260,21 +1269,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();
@ -1287,13 +1281,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.
@ -1302,7 +1293,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;
@ -1454,12 +1445,14 @@ function getDependency(node) {
!node.parent.computed &&
!(
node.parent.parent != null &&
node.parent.parent.type === 'CallExpression' &&
(node.parent.parent.type === 'CallExpression' ||
node.parent.parent.type === 'OptionalCallExpression') &&
node.parent.parent.callee === node.parent
)
) {
return getDependency(node.parent);
} else if (
// Note: we don't check OptionalMemberExpression because it can't be LHS.
node.type === 'MemberExpression' &&
node.parent &&
node.parent.type === 'AssignmentExpression'
@ -1475,20 +1468,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}`);
}