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

feat(eslint-plugin): [no-unnec-cond] check optional chaining #1315

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Dec 7, 2019

This adds type checking around the the new optional-chaining feature. With this, it becomes invalid to start an optional chain when the base value is known to be not-nullish.

// Good
declare const foo: string | null;
declare const bar: null | (() => {});
foo?.length;
foo?.slice();
bar?.();

// Bad
declare const baz: string;
declare const qux: (() => {});
baz?.length;
baz?.slice();
baz.slice?.();
qux?.();

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @jridgewell!

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.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
I think that this functionality would be better suited within the no-unnecessary-condition rule.
Would you please be able to move it in there?

@jridgewell jridgewell force-pushed the no-optional-chain-unless-nullish branch 2 times, most recently from e74bea4 to 382164b Compare December 10, 2019 06:37
@jridgewell
Copy link
Contributor Author

Done.

@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Dec 11, 2019
@bradzacher bradzacher changed the title feat(eslint-plugin): add no-optional-chain-unless-nullish rule feat(eslint-plugin): [no-unnecessary-condition] report unnecessary optional chaining Dec 11, 2019
@jridgewell jridgewell force-pushed the no-optional-chain-unless-nullish branch from 382164b to 07ad7a9 Compare December 14, 2019 09:19
@jridgewell
Copy link
Contributor Author

Updated the docs to get the tests to pass.

@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

Merging #1315 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
+ Coverage   93.92%   93.94%   +0.01%     
==========================================
  Files         131      131              
  Lines        5847     5860      +13     
  Branches     1657     1661       +4     
==========================================
+ Hits         5492     5505      +13     
  Misses        188      188              
  Partials      167      167
Impacted Files Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts 100% <100%> (ø) ⬆️

This adds type checking around the the new optional-chaining feature. With this, it becomes invalid to start an optional chain when the base value is known to be not-nullish.

```js
// Good
declare const foo: string | null;
declare const bar: null : (() => {});
foo?.length;
foo?.slice();
bar?.();

// Bad
declare const baz: string;
declare const qux: (() => {});
baz?.length;
baz?.slice();
baz.slice?.();
qux?.();
```
@jridgewell jridgewell force-pushed the no-optional-chain-unless-nullish branch from 07ad7a9 to 9d9dee1 Compare December 14, 2019 09:35
@bradzacher bradzacher mentioned this pull request Dec 16, 2019
25 tasks
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this

Nice work using nullThrows, I hadn't documented it yet.

@bradzacher bradzacher changed the title feat(eslint-plugin): [no-unnecessary-condition] report unnecessary optional chaining feat(eslint-plugin): [no-unnec-cond] check optional chaining Dec 16, 2019
@bradzacher bradzacher merged commit a2a8a0a into typescript-eslint:master Dec 16, 2019
@jridgewell jridgewell deleted the no-optional-chain-unless-nullish branch December 17, 2019 05:41
@StephanBijzitter
Copy link

Please note: the release notes use the pull request title, so I updated my config to include no-unnec-cond and it didn't very much like it. Was a bit confusing :-)

@bradzacher
Copy link
Member

bradzacher commented Dec 20, 2019

Sorry, ESLint semi enforces commit message length when doing squash merges. It's also nice to keep things short enough (72 characters) so that the commit history on github doesn't truncate the message.

It's one of the pains of using semantic commit messages; it takes a lot of the character budget to do feat(eslint-plugin):

@StephanBijzitter
Copy link

No worries, we have the same issue ourselves :-)

The way we solve it is when squash-merging a pull request, we edit the body of the commit message to make it match again

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants