[ESLint] Handle optional member chains #19275

gaearon · 2020-07-07T19:31:32Z

Follow-up to #19273.

Our algorithm treats a.b.c.d as unique paths in a many places and those parts should work the same way regardless of whether these are optional or required members. So in #19273, I removed ?. from the internal representation.

However, ideally we still want to print ?. where needed if all usages of that member in the user code used optional chaining. For example, if we use pizza?.crust and pizza?.crust.density, then we should suggest pizza?.crust in deps, but if one of them didn’t have ?, then we should suggest pizza.crust instead. Because then, at least one of them is required anyway. None of this should have any effects on the dependency logic — both variations, if typed by user, should satisfy the constraint.

The implementation is split in two stages.

It intentionally doesn’t interfere with the main algorithm that checks the dependencies. Instead, when we initially generate member paths, we also fill a Map. In that Map, we mark for each deep path whether it was a required or an optional one. By the time we have collected all dependencies, the Map will contain a mapping from the key path to a Boolean: true if all usages were optional, and false if some of them were required.

Finally, when we're ready to print our suggestions and/or fix the code, we will use that Map to pretty-print our paths. We will consult it to see if each segment should be concatenated using ?. or .. If at least one usage was required, or if that path was not found in the code, then we will default to . as a default path separator. So if you don't use ?. you won’t see it.

This adds a few tests and also fixes a bunch of existing tests to use better output.

This is split in two commits. The first one is just a bunch of renames because "optional" clashed with another concept we previously called the same way. Review commits individually.

This disambiguates "optional"/"required" because that terminology is taken by optional chaining.

codesandbox-ci · 2020-07-07T19:33:53Z

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 916b82b:

Sandbox	Source
React	Configuration

gaearon · 2020-07-07T19:36:06Z

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

              desc:
-                'Update the dependencies array to be: [pizza.crust, pizza.toppings]',
+                'Update the dependencies array to be: [pizza.crust, pizza?.toppings]',


Note: technically this could be further improved to not add ? at all because another dep didn't use it. However, I don't think it matters because at least this doesn't cause a crash. Additionally, this points your attention to unnecessary ? inside the function — since presumably if data is immutable, you shouldn't need it anyway.

gaearon · 2020-07-07T19:37:32Z

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

- * foo.(bar) -> 'foo.bar'
- * foo.bar.(baz) -> 'foo.bar.baz'
+ * foo(.)bar -> 'foo.bar'
+ * foo.bar(.)baz -> 'foo.bar.baz'


This comment didn't make sense so I fixed it.

sizebot · 2020-07-07T19:37:37Z

Details of bundled changes.

Comparing: 7ca1d86...916b82b

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+2.1%	+2.3%	82.48 KB	84.25 KB	18.9 KB	19.33 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+1.4%	🔺+1.5%	22.95 KB	23.26 KB	7.75 KB	7.87 KB	NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 916b82b

ljharb · 2020-07-07T19:39:14Z

Because then, at least one of them is required anyway.

That's not how optional chaining works, if I'm reading the OP right. a?.b.c will short-circuit, and not evaluate .c, if a is nullish (and will throw if a is non-nullish and a.b is nullish)

sizebot · 2020-07-07T19:39:41Z

Details of bundled changes.

Comparing: 7ca1d86...916b82b

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+2.1%	+2.3%	82.5 KB	84.27 KB	18.9 KB	19.34 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+1.4%	🔺+1.5%	22.96 KB	23.27 KB	7.76 KB	7.88 KB	NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 916b82b

gaearon · 2020-07-07T19:44:48Z

That's not how optional chaining works, if I'm reading the OP right. a?.b.c will short-circuit, and not evaluate .c, if a is nullish (and will throw if a is non-nullish and a.b is nullish)

Hmm. Maybe I wasn't clear or maybe I'm missing something.

I'm referring to this scenario:

useEffect(() => {
  console.log(a?.b.x)
  console.log(a?.b.x)
})

If we were to auto-fill dependencies, we'd want to suggest a?.b.x. Because suggesting a.b.x would lead to a crash.

However, with this scenario:

useEffect(() => {
  console.log(a?.b.x)
  console.log(a.b.x)
})

In this case, suggesting a?.b.x as a dep may be a bit misleading because we do use a.b.x inside, too. So it is better to call user’s attention to that by suggesting the "stricter" a.b.x as a dep. They can still edit it if they want to.

Similarly, if the user code has a?.b and a.b.x, suggesting a.b as their "union" is safer because a.b.x requires it. But if the user code has a?.b.x and a?.b then it's okay to suggest a?.b as their "union".

rickhanlonii

Looks great

rickhanlonii · 2020-07-07T19:58:56Z

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

+      errors: [
+        {
+          message:
+            "React Hook useEffect has a missing dependency: 'pizza.crust'. " +


We may want this to explain why only one is included with something like:

React Hook useEffect has a missing dependencies: 'pizza?.crust' and 'pizza.crust' (which can be reduced to a dependency on 'pizza.crust'). Either include it or remove the dependency array.

Meh. I don't think this is useful enough to clutter the message.

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

ljharb · 2020-07-07T20:20:17Z

@gaearon as long as there's no getters in the chain that might change the nullishness of properties on later evaluation, and no assignments or mutations in between the two statements, you're right :-)

gaearon · 2020-07-07T20:31:24Z

Yeah. This rule in general assumes that render-scope objects you read inside Hook callbacks are immutable POJOs or treated as such. Otherwise dependencies don't work as a concept in general.

Co-authored-by: Ricky <rickhanlonii@gmail.com>

* Rename internal variables This disambiguates "optional"/"required" because that terminology is taken by optional chaining. * Handle optional member chains * Update comment Co-authored-by: Ricky <rickhanlonii@gmail.com> Co-authored-by: Ricky <rickhanlonii@gmail.com>

gaearon added 2 commits July 7, 2020 17:51

Rename internal variables

7ea2ff2

This disambiguates "optional"/"required" because that terminology is taken by optional chaining.

Handle optional member chains

508f61b

gaearon mentioned this pull request Jul 7, 2020

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

Merged

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 7, 2020

gaearon requested review from bvaughn and rickhanlonii July 7, 2020 19:31

gaearon commented Jul 7, 2020

View reviewed changes

rickhanlonii approved these changes Jul 7, 2020

View reviewed changes

Update comment

916b82b

Co-authored-by: Ricky <rickhanlonii@gmail.com>

gaearon merged commit 7c35cb2 into facebook:master Jul 7, 2020

gaearon deleted the optt branch July 7, 2020 20:44

rickhanlonii mentioned this pull request Jul 8, 2020

Document Why a "complex expression in the dependency array" Is a Problem reactjs/react.dev#3076

Open

motiz88 mentioned this pull request Dec 23, 2022

[ESLint] Preserve optional chains in declared dependencies #25932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ESLint] Handle optional member chains #19275

[ESLint] Handle optional member chains #19275

gaearon commented Jul 7, 2020 •

edited

Loading

codesandbox-ci bot commented Jul 7, 2020 •

edited

Loading

gaearon Jul 7, 2020

gaearon Jul 7, 2020

sizebot commented Jul 7, 2020 •

edited

Loading

eslint-plugin-react-hooks

ljharb commented Jul 7, 2020 •

edited

Loading

sizebot commented Jul 7, 2020 •

edited

Loading

eslint-plugin-react-hooks

gaearon commented Jul 7, 2020 •

edited

Loading

rickhanlonii left a comment

rickhanlonii Jul 7, 2020

gaearon Jul 7, 2020

ljharb commented Jul 7, 2020

gaearon commented Jul 7, 2020

[ESLint] Handle optional member chains #19275

[ESLint] Handle optional member chains #19275

Conversation

gaearon commented Jul 7, 2020 • edited Loading

codesandbox-ci bot commented Jul 7, 2020 • edited Loading

gaearon Jul 7, 2020

Choose a reason for hiding this comment

gaearon Jul 7, 2020

Choose a reason for hiding this comment

sizebot commented Jul 7, 2020 • edited Loading

eslint-plugin-react-hooks

Size changes (stable)

ljharb commented Jul 7, 2020 • edited Loading

sizebot commented Jul 7, 2020 • edited Loading

eslint-plugin-react-hooks

Size changes (experimental)

gaearon commented Jul 7, 2020 • edited Loading

rickhanlonii left a comment

Choose a reason for hiding this comment

rickhanlonii Jul 7, 2020

Choose a reason for hiding this comment

gaearon Jul 7, 2020

Choose a reason for hiding this comment

ljharb commented Jul 7, 2020

gaearon commented Jul 7, 2020

gaearon commented Jul 7, 2020 •

edited

Loading

codesandbox-ci bot commented Jul 7, 2020 •

edited

Loading

sizebot commented Jul 7, 2020 •

edited

Loading

ljharb commented Jul 7, 2020 •

edited

Loading

sizebot commented Jul 7, 2020 •

edited

Loading

gaearon commented Jul 7, 2020 •

edited

Loading