Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [prefer-optional-chain] fixThisExpression andPrivateIdentifier errors#6028
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 onhttps://opencollective.com/typescript-eslint. |
nx-cloudbot commentedNov 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
netlifybot commentedNov 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
Uh oh!
There was an error while loading.Please reload this page.
| // private properties | ||
| 'this.#a && this.#b;', | ||
| '!this.#a || !this.#b;', | ||
| 'a.#foo && a.#foo.bar;', | ||
| '!a.#foo || !a.#foo.bar;', | ||
| 'a.#foo?.bar;', | ||
| '!a.#foo?.bar;', |
Josh-CenaNov 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
More test cases to consider:
foo().#aa.b.#anewA().#b(awaita).#b
I have no idea why this rule is so strict about what expressions it can lint, but you can in fact use private properties on all expressions, just that some may have nonsensical behaviors (e.g.(typeof a).#a). It shouldn't crash the linter anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can I fix the rule to not intentionally throw? this seems irrational to me.
It's better to not show a suggestion than to just break and do nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would love to change this rule to not intentionally throw, or at least catch the error.
The rule logic now has many combinations that could be missed and better be gracefully ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah as Brad said in#6028 (comment), I think the rule should no longer throw. Throwing is useful for us finding missing combinations but is inconvenient for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, so I misunderstood his comment
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
codecovbot commentedNov 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## main #6028 +/- ##==========================================- Coverage 91.37% 91.36% -0.01%========================================== Files 368 368 Lines 12596 12605 +9 Branches 3709 3713 +4 ==========================================+ Hits 11509 11516 +7- Misses 772 773 +1- Partials 315 316 +1
Flags with carried forward coverage won't be shown.Click here to find out more.
|
ThisExpression andPrivateIdentifierThisExpression andPrivateIdentifier errorsSomething funky is up with the Git history 🤔 |
ThisExpression andPrivateIdentifier errorsThisExpression andPrivateIdentifier errorsit looks like you might have rebased weirdly or something? it's borked the PR contents! could you please update your branch so it just contains your commits - thanks! |
e647994 to7e2185eCompare…refer-optional-chain-private-and-this-in-negated-or
…refer-optional-chain-private-and-this-in-negated-or
| if ( | ||
| [ | ||
| AST_NODE_TYPES.TSAsExpression, | ||
| AST_NODE_TYPES.AwaitExpression, | ||
| AST_NODE_TYPES.NewExpression, | ||
| ].includes(node.object.type) | ||
| ) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This feels a bit like a hack since typescript doesn't seem to think they are assignable toMemberExpressionif (node.object.type === AST_NODE_TYPES.TSAsExpression) {} shows a typescript error, but with Array#includes it does not
JoshuaKGoldbergDec 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is a bit of a hack 😄 but the root cause is a bug in our typings, I think. A node's membercan be anawait ornew. [typescript-eslint playground]
constexample={}asany;(awaitexample).prop;(awaitnewexample).prop;
For now, you can use a// @ts-expect-error and link to#6239#6192
…ate-and-this-in-negated-or
…ate-and-this-in-negated-or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Still a few comments please 😇
…ate-and-this-in-negated-or
…ate-and-this-in-negated-or
…ate-and-this-in-negated-or
fix the merge conflicts and we can get this in! |
…refer-optional-chain-private-and-this-in-negated-or# Conflicts:#packages/eslint-plugin/src/rules/prefer-optional-chain.ts#packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts
@bradzacher Thanks, fixed the conflict |
…ate-and-this-in-negated-or
…ate-and-this-in-negated-or
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.48.2/5.50.0) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.48.2/5.50.0) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31)[Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0)##### Bug Fixes- **eslint-plugin:** \[ban-ts-comment] counts graphemes instead of `String.prototype.length` ([#​5704](typescript-eslint/typescript-eslint#5704)) ([09d57ce](typescript-eslint/typescript-eslint@09d57ce))- **eslint-plugin:** \[prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors ([#​6028](typescript-eslint/typescript-eslint#6028)) ([85e783c](typescript-eslint/typescript-eslint@85e783c))- **eslint-plugin:** \[prefer-optional-chain] fixer produces wrong logic ([#​5919](typescript-eslint/typescript-eslint#5919)) ([b0f6c8e](typescript-eslint/typescript-eslint@b0f6c8e)), closes [#​1438](typescript-eslint/typescript-eslint#1438)##### Features- **eslint-plugin:** add `key-spacing` rule extension for interface & type declarations ([#​6211](typescript-eslint/typescript-eslint#6211)) ([67706e7](typescript-eslint/typescript-eslint@67706e7))### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23)[Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0)##### Features- **eslint-plugin:** \[naming-convention] add support for `#private` modifier on class members ([#​6259](typescript-eslint/typescript-eslint#6259)) ([c8a6d80](typescript-eslint/typescript-eslint@c8a6d80))#### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16)**Note:** Version bump only for package [@​typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin)#### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09)**Note:** Version bump only for package [@​typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin)</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31)[Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23)[Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)#### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)#### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - 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.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTQuMSIsInVwZGF0ZWRJblZlciI6IjM0LjExOS4yIn0=-->Co-authored-by: cabr2-bot <cabr2.help@gmail.com>Reviewed-on:https://codeberg.org/Calciumdibromid/CaBr2/pulls/1757Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
PR Checklist
Overview
This only prevents the plugin from crashing, I tried to support the syntax as well but got into a weird behavior from the fixer reverting the fix