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

Jump to bottom

[eslint-plugin-react-hooks] fix: optional chaining safety #30989

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+94 −37
Open

[eslint-plugin-react-hooks] fix: optional chaining safety #30989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cda8b01
fix: optional chaining safety
tuttleb Sep 17, 2024
70a93ba
fix: use parent to determine if property should be accessed optionally
tuttleb Sep 17, 2024
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
86 changes: 75 additions & 11 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 @@ -1627,6 +1627,70 @@ const tests = {
},
],
},
{
code: normalizeIndent`
function MyComponent(props) {
useEffect(() => {
if(props.foo?.bar) {
console.log(props.foo.bar);
}
}, [])
}
`,
errors: [
{
message:
"React Hook useEffect 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) {
useEffect(() => {
if(props.foo?.bar) {
console.log(props.foo.bar);
}
}, [props.foo?.bar])
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function ParentImpliedOptional(props) {
useEffect(() => {
if(props.foo?.bar) {
console.log(props.foo.baz);
}
}, [])
}
`,
errors: [
{
message:
"React Hook useEffect has missing dependencies: 'props.foo?.bar' and 'props.foo?.baz'. " +
'Either include them or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [props.foo?.bar, props.foo?.baz]',
output: normalizeIndent`
function ParentImpliedOptional(props) {
useEffect(() => {
if(props.foo?.bar) {
console.log(props.foo.baz);
}
}, [props.foo?.bar, props.foo?.baz])
}
`,
},
],
},
],
},
{
code: normalizeIndent`
function MyComponent() {
Expand Down Expand Up @@ -3938,7 +4002,7 @@ const tests = {
errors: [
{
message:
"React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " +
"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.",
Expand Down Expand Up @@ -4998,7 +5062,7 @@ const tests = {
{
message:
'React Hook useCallback has unnecessary dependencies: ' +
"'MutableStore.hello.world', 'global.stuff', 'props.foo', 'x', 'y', and 'z'. " +
"'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.",
Expand Down Expand Up @@ -7921,19 +7985,19 @@ const testsTypescript = {
errors: [
{
message:
"React Hook useEffect has missing dependencies: 'pizza.crust' and 'pizza?.toppings'. " +
"React Hook useEffect has missing dependencies: 'pizza?.crust' and 'pizza?.toppings'. " +
'Either include them or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [pizza.crust, pizza?.toppings]',
desc: 'Update the dependencies array to be: [pizza?.crust, pizza?.toppings]',
output: normalizeIndent`
function MyComponent() {
const pizza = {};

useEffect(() => ({
crust: pizza.crust,
toppings: pizza?.toppings,
}), [pizza.crust, pizza?.toppings]);
}), [pizza?.crust, pizza?.toppings]);
}
`,
},
Expand All @@ -7955,19 +8019,19 @@ const testsTypescript = {
errors: [
{
message:
"React Hook useEffect has a missing dependency: 'pizza.crust'. " +
"React Hook useEffect has a missing dependency: 'pizza?.crust'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [pizza.crust]',
desc: 'Update the dependencies array to be: [pizza?.crust]',
output: normalizeIndent`
function MyComponent() {
const pizza = {};

useEffect(() => ({
crust: pizza?.crust,
density: pizza.crust.density,
}), [pizza.crust]);
}), [pizza?.crust]);
}
`,
},
Expand All @@ -7989,19 +8053,19 @@ const testsTypescript = {
errors: [
{
message:
"React Hook useEffect has a missing dependency: 'pizza.crust'. " +
"React Hook useEffect has a missing dependency: 'pizza?.crust'. " +
'Either include it or remove the dependency array.',
suggestions: [
{
desc: 'Update the dependencies array to be: [pizza.crust]',
desc: 'Update the dependencies array to be: [pizza?.crust]',
output: normalizeIndent`
function MyComponent() {
const pizza = {};

useEffect(() => ({
crust: pizza.crust,
density: pizza?.crust.density,
}), [pizza.crust]);
}), [pizza?.crust]);
}
`,
},
Expand Down
45 changes: 19 additions & 26 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 @@ -414,6 +414,10 @@ export default {
// Get dependencies from all our resolved references in pure scopes.
// Key is dependency string, value is whether it's stable.
const dependencies = new Map();
// Keeps track of paths that have optional chaining
// Keys is a path string, value is whether or not there are optional
// direct descendants
// Example: foo.bar?.baz -> { "foo": false, "foo.bar": true }
const optionalChains = new Map();
gatherDependenciesRecursively(scope);

Expand Down Expand Up @@ -712,7 +716,7 @@ export default {
try {
declaredDependency = analyzePropertyChain(
declaredDependencyNode,
null,
optionalChains,
);
} catch (error) {
if (/Unsupported node type/.test(error.message)) {
Expand Down Expand Up @@ -909,9 +913,9 @@ export default {
let finalPath = '';
for (let i = 0; i < members.length; i++) {
if (i !== 0) {
const pathSoFar = members.slice(0, i + 1).join('.');
const isOptional = optionalChains.get(pathSoFar) === true;
finalPath += isOptional ? '?.' : '.';
const parentPath = members.slice(0, i).join('.');
const isParentOptional = optionalChains.get(parentPath) === true;
finalPath += isParentOptional ? '?.' : '.';
}
finalPath += members[i];
}
Expand Down Expand Up @@ -1709,19 +1713,18 @@ function getDependency(node) {
* It just means there is an optional member somewhere inside.
* This particular node might still represent a required member, so check .optional field.
*/
function markNode(node, optionalChains, result) {
function markNode(node, optionalChains, object, property) {
const result = `${object}.${property}`;
if (optionalChains) {
if (node.optional) {
// We only want to consider it optional if *all* usages were optional.
if (!optionalChains.has(result)) {
// Mark as (maybe) optional. If there's a required usage, this will be overridden.
optionalChains.set(result, true);
}
} else {
// Mark as required.
optionalChains.set(result, false);
optionalChains.set(object, true);
} else if (!optionalChains.has(object)) {
// Mark as (maybe) required. We only want to consider it required if *no* usages were optional. If
// there's an optional usage, this will be overriden.
optionalChains.set(object, false);
}
}
return result;
}

/**
Expand All @@ -1734,23 +1737,15 @@ function markNode(node, optionalChains, result) {
function analyzePropertyChain(node, optionalChains) {
if (node.type === 'Identifier' || node.type === 'JSXIdentifier') {
const result = node.name;
if (optionalChains) {
// Mark as required.
optionalChains.set(result, false);
}
return result;
} else if (node.type === 'MemberExpression' && !node.computed) {
const object = analyzePropertyChain(node.object, optionalChains);
const property = analyzePropertyChain(node.property, null);
const result = `${object}.${property}`;
markNode(node, optionalChains, result);
return result;
return markNode(node, optionalChains, object, property);
} else if (node.type === 'OptionalMemberExpression' && !node.computed) {
const object = analyzePropertyChain(node.object, optionalChains);
const property = analyzePropertyChain(node.property, null);
const result = `${object}.${property}`;
markNode(node, optionalChains, result);
return result;
return markNode(node, optionalChains, object, property);
} else if (node.type === 'ChainExpression' && !node.computed) {
const expression = node.expression;

Expand All @@ -1760,9 +1755,7 @@ function analyzePropertyChain(node, optionalChains) {

const object = analyzePropertyChain(expression.object, optionalChains);
const property = analyzePropertyChain(expression.property, null);
const result = `${object}.${property}`;
markNode(expression, optionalChains, result);
return result;
return markNode(expression, optionalChains, object, property);
} else {
throw new Error(`Unsupported node type: ${node.type}`);
}
Expand Down