Fix ExhaustiveDeps ESLint rule throwing with optional chaining

[lencioni]

Summary

Certain code patterns using optional chaining syntax causes
eslint-plugin-react-hooks to throw an error.

We can avoid the throw by adding some guards. I didn't read through the
code to understand how it works, I just added a guard to every place
where it threw, so maybe there is a better fix closer to the root cause
than what I have here.

In my test case, I noticed that the optional chaining that was used in
the code was not included in the suggestions description or output,
but it seems like it should be. This might make a nice future
improvement on top of this fix, so I left a TODO comment to that effect.

Fixes #19243

Test Plan

yarn test --watch eslint-plugin

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 44b93b6:

Sandbox	Source
React	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 44b93b6

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 44b93b6

[gaearon]

I didn't read through the code to understand how it works, I just added a guard to every place where it threw, so maybe there is a better fix closer to the root cause than what I have here.

I think at the very least we should look at why those items weren't present in those Maps. Perhaps the key is wrong? Then we should fix the key.

[lencioni]

It looks like there is a mismatch between the keys in the dependencies Map and the keys in missingDependencies. The Map includes the optional chaining in its keys (e.g. pizza?.toppings) while the missingDependencies array does not (e.g. pizza.toppings).

[gaearon]

Can we think of a fix that would address the root of the problem?

[lencioni]

Yep, looking into that now. In b0533fe it looks like the missingDependencies is normalized to remove the ? bits, so I suppose we could do the same for the dependencies Map keys, which I think would just mean using the normalizedPath instead of path in scanTreeRecursively.

Oh wait, nevermind that's a different code path. I think that Map must be built somewhere else.

[lencioni]

I've poked around a bit and I'm not sure what the best solution is yet. One way to fix this would be to normalize the paths in the dependencies Map like this maybe:

--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -561,7 +561,7 @@ export default {
             const isStatic =
               memoizedIsStaticKnownHookValue(resolved) ||
               memoizedIsFunctionWithoutCapturedValues(resolved);
-            dependencies.set(dependency, {
+            dependencies.set(dependency.replace(/\?/g, ''), {
               isStatic,
               references: [reference],
             });

but the downside is the error messages and suggestions will drop the optional chaining, which is not ideal.

I started to investigate fixing it on the other end (so that the missingDependencies Set includes the optional chaining) but that train ended up getting deeper than I have time to investigate at the moment (missingDependencies is built from depTree which comes from createDepTree which ends up normalizing the paths somehow--I think possibly by getChildByKey).

I can modify this PR to normalize out the optional chaining, or leave it as is. Anything more involved I'll have to set this aside for a while. Or perhaps @yanneves or @fredvollmer can offer some better insights, since it seems they've also been in this part of the codebase recently?

[bvaughn]

Thanks!👍

[gaearon]

@bvaughn Any particular reason you merged this fix? It doesn't address the root of the problem and I'm worried we'll just get more confusing reports later on.

[bvaughn]

Wow. I feel like a clown. I did not see any of the above running discussion between the two of you. I'm not sure how I overlooked it but I somehow did.

I was looking through recent issues and PRs (because of oncall)- saw this, tested it, thought it looked reasonable, so I merged it.

I am sorry!

[gaearon]

#19273