feat(eslint-plugin): [no-unused-expressions] extend for optional chaining

[Nizarius]

Fixes #1138

To note: this PR depends on #1169, so it's better to watch after first one

[typescript-eslint]

Thanks for the PR, @Nizarius!

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.

[bradzacher]

it looks like you've copied most of the code from the existing rule implementation.
May I suggest that you instead build on top of the existing implementation?

Duplicating the implementation means we have to keep it in sync with eslint, which is a huge maintenance burden.

You could do something similar to
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/camelcase.ts

I.e. - defer most of the processing to the base rule's handler, but add handling which will handle optional chaining:

const rules = baseRule.create(context);
ExpressionStatement(node) {
  if (isValidOptionalChain(node)) {
    return;
  }
  
  rules.ExpressionStatement(node);
}

[Nizarius]

@bradzacher yeah, great idea! I will do it, but it looks like I would still need a lot of duplicating code as I can't get private functions from eslint rule.

[Nizarius]

@bradzacher done. Can you tell me how I should add docs to that rule (just copy it from eslint or other way?)

[bradzacher]

Don't forget to add a doc for the new rule.
Have a look at the docs for semi
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/semi.md

Essentially with rule extensions now, we like our extension rule docs to just mention the changes it makes to the rule, and then link out to the original eslint docs

[Nizarius]

@bradzacher added docs and new tests. Could you explain to me how I should update documentation parsing to avoid 'Incorrect line number for' errors?

Also, should I add this rule to recommended ones to prevent errors with base configuration?

[bradzacher]

Could you explain to me how I should update documentation parsing to avoid 'Incorrect line number for' errors?

The rule needs to be listed in the plugin readme. The table should be sorted alphabetically.

Also, should I add this rule to recommended ones to prevent errors with base configuration?

Changes to the recommended config are considered breaking changes, so we cannot do that for now.
Feel free to raise a separate tracking issue so we can keep track of doing this when we start on a 3.0.0 release.

[bradzacher]

LGTM - thanks for this.

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   93.96%   93.98%   +0.01%     
==========================================
  Files         120      121       +1     
  Lines        5207     5217      +10     
  Branches     1443     1444       +1     
==========================================
+ Hits         4893     4903      +10     
  Misses        179      179              
  Partials      135      135

Impacted Files	Coverage Δ
...s/eslint-plugin/src/rules/no-unused-expressions.ts	100% <100%> (ø)
packages/eslint-plugin/src/rules/index.ts	100% <100%> (ø)	⬆️

[glen-84]

@JamesHenry This isn't in the release notes?

[bradzacher]

@glen-84 it was released earlier - in 2.7
https://github.com/typescript-eslint/typescript-eslint/releases/tag/v2.7.0

[glen-84]

🤦‍♂ I must have missed that.