fix(eslint-plugin-react-hooks): Support optional chaining when accessing prototype method inside useCallback and useMemo #19061#19062
Conversation
|
Hi @fredvollmer! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
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 fb8040d:
|
|
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 d9a6766:
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
…ing prototype method react#19061
| satisfyingPaths.add(path); | ||
| // Here we only want to compare the "normalized" path, without any optional chaining ("?.") operator | ||
| // foo?.bar -> foo.bar | ||
| satisfyingPaths.add(path.replace(/\?$/, '')); |
There was a problem hiding this comment.
The same change should be made below when adding path to missingPaths (line 1304). Version 4.04 is now getting an eslint error:
ESLint stack trace:
[Error - 4:20:28 PM] TypeError: Cannot read property 'references' of undefined
Issue #19043
Your same change to missingPaths fixes that error.
missingPaths.add(path.replace(/\?$/, ''));
There was a problem hiding this comment.
Great catch! Just made this change.
|
Do we know when this might get merged? |
|
Hi! Looks like the change is approved. Is there a reason why it's not merged yet? |
|
Running into this error too. |
|
Out in eslint-plugin-react-hooks@4.0.5 |
|
On eslint-plugin-react-hooks@4.0.5 with ESLint 7.3.1 is not working for me... The error "TypeError: Cannot read property 'references' of undefined" appears. Any thoughts? |
|
File a new issue with code causing it. |
|
Locking so we don't have to track discussion in a merged PR. If there is a new problem, please file a new issue. |
|
Optional chaining problems should be fixed in |
Relates to #19061
Summary
This fix ensures that optional chaining can be used to access a prototype method of an object, inside a
useCallbackoruseEffecthook.For example, assume we use optional chaining to call the
toString()method of an object:In such a case,
foo, notfoo?.toString, should be added to the dependencies array of theuseCallbackoruseMemohooks. However, doing so currently throws a false "unnecessary dependencies" warning.Test Plan
Additional tests were added to cover using the optional chaining operator in the
useCallbackanduseMemohooks, when only a prefix of the object path is provided (as it would be if a prototype method is being referenced in the callback.)