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

fix(eslint-plugin): [prefer-optional-chain] handle more cases #1261

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Nov 26, 2019

I imported my implementation to the fb codebase and ran it (because I figured this rule isn't actually TS specific), and I found a few cases that I missed in the original implementation.

To better handle the cases, I significantly reduced the "allowed" scope of the rule by only allowing certain AST structures (see the isValidChainTarget function).

Changes:

at detection time:

  1. no longer false-positives on chains where the first variable name is a substring of the next in the chain:
    • foo && fooBar.baz
  2. now can detect chains with some optionals already:
    • foo && foo.bar?.baz
  3. no longer detcets arbitrarily "complex" computed cases, such as:
    foo && foo[bar as string] && foo[bar as string].baz, or
    foo && foo[1 + 2] && foo[1 + 2].baz.
    • I realised that in order to "normalise" the text for comparison, the rule needs to know how to normalise every allowed node.
    • There's probably a better algorithm than just comparing text directly that removes this limitation.

at fix time:

  1. no longer modifies call expression arguments (fixes a bug where required spaces in unary operators and JSX was stripped):
    • foo && foo.map(bar => <Baz bar={bar} />)
      • fix to foo?.map(bar => <Baz bar={bar} />)
    • foo && foo.map(bar => typeof bar)
      • fix to foo?.map(bar => typeof bar)
    • foo && foo.bar(/* comment */a, )
      • fix to foo?.bar(/* comment */a, )
  2. No longer strips spaces from computed member expression literals:
    • foo && foo['bar baz']
      • fix to foo?.['bar baz']
  3. No longer drops trailing binary expressions:
    • foo != null && foo.bar != null
      • fix to foo?.bar != null
    • foo && foo.bar != null && baz
      • fix to foo?.bar != null && baz

@bradzacher bradzacher added the bug Something isn't working label Nov 26, 2019
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

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.

@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #1261 into master will decrease coverage by 0.02%.
The diff coverage is 93.42%.

@@            Coverage Diff             @@
##           master    #1261      +/-   ##
==========================================
- Coverage    94.1%   94.07%   -0.03%     
==========================================
  Files         130      131       +1     
  Lines        5681     5737      +56     
  Branches     1596     1616      +20     
==========================================
+ Hits         5346     5397      +51     
- Misses        182      184       +2     
- Partials      153      156       +3
Impacted Files Coverage Δ
packages/eslint-plugin/src/util/misc.ts 88% <100%> (ø) ⬆️
packages/eslint-plugin/src/util/nullThrows.ts 100% <100%> (ø)
packages/eslint-plugin/src/util/index.ts 100% <100%> (ø) ⬆️
...s/eslint-plugin/src/rules/prefer-optional-chain.ts 95.14% <92.18%> (-4.86%) ⬇️
...nt-plugin/src/rules/no-untyped-public-signature.ts 100% <0%> (ø) ⬆️

@bradzacher bradzacher changed the title fix(eslint-plugin): handle some unexpected cases fix(eslint-plugin): [prefer-optional-chain] handle more cases Nov 26, 2019
@glen-84
Copy link
Contributor

glen-84 commented Nov 26, 2019

const x = {y: 0};
if (x["y"] !== undefined && x["y"] !== null) {
    //
}

... is currently being reported, fixed to:

const x = {y: 0};
if (x["y"]) {
    //
}

This doesn't seem right at all. My real-world example actually looks like this:

const field = `relatedArticleIds${i}`;

if (data[field] !== undefined && data[field] !== null) {
    relatedArticleIds.push(data[field]);
}

(i.e. it uses a dynamic index)

The fix doesn't actually use optional chaining, and the code is not equivalent (checking for not undefined and not null vs checking for truthy).

Will this be fixed here (has it already been?), or should I open a separate issue for it?

@bradzacher
Copy link
Member Author

@glen-84 - that issue is fixed by this PR.
https://github.com/typescript-eslint/typescript-eslint/pull/1261/files#diff-9e91bdee9914244f3ff940760d3f59bfR119

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants