feat(eslint-plugin): [prefer-optional-chain] support logical with empty object by omril1 · Pull Request #4430 · typescript-eslint/typescript-eslint

omril1 · 2022-01-10T15:56:35Z

Adds empty object pattern ((foo || {}).bar or (foo ?? {}).bar) as a match for prefer-optional-chain rule

PR Checklist

Addresses an existing issue: fixes [prefer-optional-chain] Suggest (foo || {}).bar as a valid match for the rule #4395
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Adds support for suggesting optional chaining refactor when an empty object is used to mimic optional chaining in addition to the existing && pattern ((foo || {}).bar and (foo ?? {}).bar are roughly equivalent to foo && foo.bar or foo?.bar)

Notes

The feature does not uses type information
The feature does not handle function calls in this way as (foo || {}).bar() can cause a runtime error (unlike the match for &&).

Empty object as optional chaining

nx-cloud · 2022-01-10T15:56:37Z

☁️ Nx Cloud Report

CI ran the following commands for commit 44236bb. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 43 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-01-10T15:56:40Z

Thanks for the PR, @omril1!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-01-10T15:56:46Z

✅ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: 44236bb

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/6234ebec3a1fff0008fe7c9d

😎 Browse the preview: https://deploy-preview-4430--typescript-eslint.netlify.app

codecov · 2022-01-10T16:06:30Z

Codecov Report

Merging #4430 (44236bb) into main (8ec05be) will increase coverage by 0.05%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #4430      +/-   ##
==========================================
+ Coverage   94.25%   94.30%   +0.05%     
==========================================
  Files         151      151              
  Lines        7971     7991      +20     
  Branches     2573     2578       +5     
==========================================
+ Hits         7513     7536      +23     
+ Misses        262      258       -4     
- Partials      196      197       +1

Flag	Coverage Δ
unittest	`94.30% <95.23%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/eslint-plugin/src/rules/prefer-optional-chain.ts	`94.11% <95.23%> (+0.17%)`	⬆️
...es/eslint-plugin/src/util/getOperatorPrecedence.ts	`42.85% <0.00%> (+11.42%)`	⬆️

Empty object as optional chaining

const sourceCode already available in the upper scope

JoshuaKGoldberg

Looks great so far, thanks!

Just requesting changes on a bit more testing please ❤️

JoshuaKGoldberg · 2022-01-24T00:48:41Z

  create(context) {
    const sourceCode = context.getSourceCode();
    return {
+      'LogicalExpression[operator=||]'(node: TSESTree.LogicalExpression): void {


The feature does not handle function calls in this way as (foo || {}).bar() can cause a runtime error.

Hmm, interesting point. My hunch would be that we probably want to handle function calls for cases like this:

(someContainer || {}).hasOwnProperty("someKey")

...but I wouldn't consider it a blocker / must-have for this PR. Up to you!

@JoshuaKGoldberg Care to re-review this? I don't think I'm going to add object built-ins in the PR

You got it, no problem!

- Wrap left node if it's ternary or await expressions - Early exit if left node can't be a nullish object - Add More UT cases

JoshuaKGoldberg

Looks great to me, thanks so much for the thorough implementation & discussion! 🎉

@bradzacher is all over the history for this already-complex file so I'll just want to get a second opinion before merging.

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>

Catch more cases

omril1 · 2022-02-23T19:46:26Z

@bradzacher & @JoshuaKGoldberg any update about this PR?

bradzacher · 2022-02-23T20:15:51Z

it's on the review queue for when the volunteer maintainers have time to review PRs!

bradzacher

looking good! Just one comment

bradzacher · 2022-03-18T17:59:10Z

+                // Any node that is made of an operator with higher or equal precedence,
+                const maybeWrappedLeftNode =
+                  leftNode.type === AST_NODE_TYPES.BinaryExpression ||
+                  leftNode.type === AST_NODE_TYPES.TSAsExpression ||
+                  leftNode.type === AST_NODE_TYPES.UnaryExpression ||
+                  leftNode.type === AST_NODE_TYPES.LogicalExpression ||
+                  leftNode.type === AST_NODE_TYPES.ConditionalExpression ||
+                  leftNode.type === AST_NODE_TYPES.AwaitExpression


We have a module which supports getting the precedence!
We should use this so that we handle all of the cases properly, considering it's a fixer.

It's a tiny bit cumbersome because we need to get the ts node first.

const parserServices = util.getParserServices(context, true); // ... function isHigherPrecedence() { const logicalTsNode = parserServices.esTreeNodeToTSNodeMap.get(node); const logicalPrecedence = util.getOperatorPrecedence(node.kind, node.operatorToken.kind); const leftTsNode = parserServices.esTreeNodeToTSNodeMap.get(leftNode); const operator = isBinaryExpression(node) ? node.operatorToken.kind : ts.SyntaxKind.Unknown; const leftPrecedence = util.getOperatorPrecedence(leftTsNode.kind, operator); return leftPrecedence > logicalPrecedence; }

(Eventually we should make this API work with ESTree nodes so it's nicer)

OK, I did that with a slight alteration because it didn't work (even after fixing the variable names from the snippet)

* Use util.getOperatorPrecedence instead of hard coding the precedence

bradzacher

LGTM - thanks for thsi!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.15.0` -> `5.16.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.15.0/5.16.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.15.0` -> `5.16.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.15.0/5.16.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.16.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5160-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5150v5160-2022-03-21) [Compare Source](typescript-eslint/typescript-eslint@v5.15.0...v5.16.0) ##### Bug Fixes - **eslint-plugin:** \[consistent-type-assertions] enforce assertionStyle for `const` assertions ([#4685](typescript-eslint/typescript-eslint#4685)) ([8ec05be](typescript-eslint/typescript-eslint@8ec05be)) ##### Features - **eslint-plugin:** \[prefer-optional-chain] support logical with empty object ([#4430](typescript-eslint/typescript-eslint#4430)) ([d21cfe0](typescript-eslint/typescript-eslint@d21cfe0)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.16.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5160-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5150v5160-2022-03-21) [Compare Source](typescript-eslint/typescript-eslint@v5.15.0...v5.16.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1236 Reviewed-by: 6543 <6543@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

avaly · 2022-03-28T09:13:55Z

This change seems to be a breaking change because this rule now requires type information.

Error: Error while loading rule '@typescript-eslint/prefer-optional-chain': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.

bradzacher · 2022-03-28T10:25:18Z

@avaly As per our contributing guide. Please don't comment on closed PRs.
Instead raise an issue and fill out the template.

feat(eslint-plugin): issue 4395

a5d7ec7

Empty object as optional chaining

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

a1dcc4d

bradzacher changed the title ~~feat(eslint-plugin): Issue 4395 - Empty object as match for optional chaining rule~~ feat(eslint-plugin): [prefer-optional-chain] support logical with empty object Jan 11, 2022

bradzacher added the enhancement New feature or request label Jan 11, 2022

Omri Luzon and others added 2 commits January 11, 2022 12:19

feat(eslint-plugin): issue 4395

a5823d3

Empty object as optional chaining

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

65392a8

omril1 marked this pull request as ready for review January 11, 2022 10:23

omril1 added 2 commits January 13, 2022 09:02

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

c788fcb

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

bd59eb5

dimaMachina reviewed Jan 19, 2022

View reviewed changes

Comment thread packages/eslint-plugin/src/rules/prefer-optional-chain.ts Outdated

dimaMachina reviewed Jan 19, 2022

View reviewed changes

Comment thread packages/eslint-plugin/src/rules/prefer-optional-chain.ts Outdated

omril1 and others added 2 commits January 19, 2022 21:38

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

6002a5c

fix(eslint-plugin): cr comment

aac5eb1

const sourceCode already available in the upper scope

JoshuaKGoldberg reviewed Jan 24, 2022

View reviewed changes

Comment thread packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts Outdated

JoshuaKGoldberg requested changes Jan 24, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 24, 2022

Omri Luzon and others added 6 commits January 25, 2022 00:18

fix(eslint-plugin): more UT and handle ternary

155e936

- Wrap left node if it's ternary or await expressions - Early exit if left node can't be a nullish object - Add More UT cases

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

13f6e95

fix(eslint-plugin): remove comment

7f907c5

fix(eslint-plugin): prefer optional chaining over ?? {}).

d9c8120

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

4065bfe

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

3249b85

omril1 requested a review from JoshuaKGoldberg February 2, 2022 14:55

JoshuaKGoldberg previously approved these changes Feb 3, 2022

View reviewed changes

Comment thread packages/eslint-plugin/src/rules/prefer-optional-chain.ts Outdated

Comment thread packages/eslint-plugin/src/rules/prefer-optional-chain.ts

Legit nitpick

914d2f9

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>

omril1 dismissed JoshuaKGoldberg’s stale review via 914d2f9 February 9, 2022 21:29

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

6836a74

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 9, 2022

bradzacher requested changes Feb 9, 2022

View reviewed changes

Comment thread packages/eslint-plugin/src/rules/prefer-optional-chain.ts Outdated

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Feb 9, 2022

Omri Luzon and others added 2 commits February 10, 2022 23:51

fix(eslint-plugin): prefer optional chaining

d1650b0

Catch more cases

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

f355e92

omril1 requested a review from bradzacher February 12, 2022 20:40

omril1 added 2 commits February 13, 2022 00:29

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

75884fb

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

aa33afb

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 14, 2022

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

3dc29b3

bradzacher requested changes Mar 18, 2022

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 18, 2022

omril1 and others added 2 commits March 18, 2022 20:35

Merge branch 'main' into feat/issue-4395-empty-object-optional-chaining

2e06f7a

feat(eslint-plugin): cr util.getOperatorPrecedence

44236bb

* Use util.getOperatorPrecedence instead of hard coding the precedence

omril1 requested a review from bradzacher March 18, 2022 20:33

bradzacher approved these changes Mar 18, 2022

View reviewed changes

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 18, 2022

bradzacher merged commit d21cfe0 into typescript-eslint:main Mar 18, 2022

avaly mentioned this pull request Mar 28, 2022

[prefer-optional-chain] Breaking change in v5.16.0 - requires type information #4755

Closed

3 tasks

typescript-eslint locked and limited conversation to collaborators Mar 28, 2022

omril1 deleted the feat/issue-4395-empty-object-optional-chaining branch June 4, 2022 08:17

Uh oh!

Conversation

omril1 commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Notes

Uh oh!

nx-cloud Bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

typescript-eslint Bot commented Jan 10, 2022

Uh oh!

netlify Bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

omril1 Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

omril1 commented Feb 23, 2022

Uh oh!

bradzacher commented Feb 23, 2022

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

bradzacher Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

omril1 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

avaly commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradzacher commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

omril1 commented Jan 10, 2022 •

edited

Loading

nx-cloud Bot commented Jan 10, 2022 •

edited

Loading

netlify Bot commented Jan 10, 2022 •

edited

Loading

codecov Bot commented Jan 10, 2022 •

edited

Loading

avaly commented Mar 28, 2022 •

edited

Loading

bradzacher commented Mar 28, 2022 •

edited

Loading