[eslint-plugin-react-hooks] fix: optional chaining safety #30989
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This partially fixes #23248
There are are few circumstances where eslint-plugin-react-hooks will generate dependency arrays that unsafely access properties, even if those properties are safely accessed within the hook.
Problem 1: Preferring required over optional property access
Example 1:
props.foo.bar
is being accessed safely in the hook because of the if statement, but eslint-plugin-react-hooks will suggest a dependency array of[props.foo.bar]
which will unsafely access the property resulting in an error likeUncaught TypeError: Cannot read properties of undefined (reading 'bar')
when foo is undefinedRight now we always prefer treating a property as required if it appears both with and without optional chaining. In other words, in the example above
props.foo.bar
always wins overprops.foo?.bar
The fix for this problem is mostly contained in the
markNode
function where optional access now takes precedence over required access.Problem 2: Tracking the property as optional, not the parent object
Example 2:
This hook safely accesses
props.foo.baz
(albeit in a strange way). In the current implementation we would generate a dependencies array of[props.foo?.bar, props.foo.baz]
, which doesn't safely accessbaz
and could again lead to type errors coming from the dependencies array. The issue here is that we are tracking thatprops.foo.bar
is optional when we should instead be tracking that the object,props.foo
, should have all access done optionally.The fix here is adjusting the
optionalChains
map to be a map of objects that have optional property access off of them, rather than a map of properties that are accessed optionally. This resulted in changes tomarkNode
as well asformatDependency
For example 2, the optionalChains map before this change would look like:
and after this change it should look like:
So before we would explicitly say that only
props.foo.bar
needed to be accessed optionally, but now we are saying that any property off ofprops.foo
needs to be accessed optionally.Problem 3: Not tracking optionalChains in declaredDependencies
In existing test cases like this we have a deps array like
[ref1?.current, ref2?.current, props.someOtherRefs, props.color]
and expect an error message containingReact Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'.
Notably the optional chaining is missing in the error message.The fix here was to pass
optionalChains
in when callinganalyzePropertyChain
for a declared dependency nodeWhy I'm considering this a partial fix
We don't use optional chaining to determine if a dependency is missing or invalid, so this change will not force updates of existing dependency arrays or even alert you that there is potentially unsafe lack of optional chaining in an existing otherwise valid dependency array. I think this is the right tradeoff though, because it would be a pretty major and disruptive change to invalidate existing dependency arrays.
How did you test this change?
I added two new tests, matching example 1 and example 2 described above in this pull request. I also updated some existing tests to account for these changes. Specifically the pizza related tests are the most meaningfully changed
For example
before we would want a dependency array of
[pizza.crust, pizza?.toppings]
, but now we are saying that properties onpizza
must be accessed with optional chaining, so we would want[pizza?.crust, pizza?.toppings]