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.

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

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

Merged
merged 7 commits into from
Jul 7, 2020

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 7, 2020

We've merged a bunch of PRs which added support for optional chaining to the ESLint rule about dependencies (#18820, #19008, #19062, #19260). However there was no single cohesive strategy behind them, and they led to crashes and edge cases because they attacked the symptoms rather than the root cause (the algorithm wasn’t designed to deal with optional chaining). There was no attempt to rethink the algorithm so the interim solutions have gradually made it broken and inconsistent. However, we did get new tests from them so they were still useful PRs. We just need to rethink our strategy.

As a first step I want to simplify this by getting back to where we were and removing the code that breaks algorithm’s assumptions. In particular, I am treating all foo?.bar as foo.bar from the dependency tracking perspective. The downside is that our error messages and fix suggestions will suggest regular chaining (but they will also be okay with optional chaining — so you can always fix them manually). The upside is that we can reason about what’s happening again because the inconsistencies are removed, and the algorithm only deals with regular property paths, just like before.

This fixes a bunch of bugs, for which I’ve added regression tests.

As a follow-up to this, I think we should be able to figure out a simple solution that will preserve optional chaining where it makes sense in the messages and suggestions. However, I think this first step is still useful to get in as a base.

Best reviewed as individual commits

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 7, 2020

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 26271d4:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Jul 7, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 26271d4

@sizebot
Copy link

sizebot commented Jul 7, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 26271d4

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strategy makes sense, and all of the added tests will help re-add support for this the right way 👍

@gaearon gaearon merged commit 7ca1d86 into facebook:master Jul 7, 2020
@gaearon gaearon deleted the opt-chain branch July 7, 2020 16:40
@bvaughn
Copy link
Contributor

bvaughn commented Jul 7, 2020

Nice cleanup!

@radko93
Copy link

radko93 commented Jul 7, 2020

Hey! It would be great to have a new version published with this, right now npm v4.0.5 breaks linters in projects.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 7, 2020

@radko93 Thanks, we're aware from the reports. :-) I am working on a proper fix.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 7, 2020

#19275

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 7, 2020

[email protected] is out with fixes.

trueadm pushed a commit to trueadm/react that referenced this pull request Jul 8, 2020
…cebook#19273)

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

This reverts commit 0f84b0f.

* Re-add a test from facebook#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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants