Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] fix
ThisExpression
andPrivateIdentifier
errors #6028fix(eslint-plugin): [prefer-optional-chain] fix
ThisExpression
andPrivateIdentifier
errors #6028Changes from 4 commits
71cb502
5acc1af
a3b6eda
a68b13a
7e2185e
1a30e94
21cfbd2
0ae6c45
ecfacff
3ce9b0a
c3c458e
9c15f28
ab70949
e58dab9
12effef
0afc516
8efa51d
57b86c1
99528f0
cbb5168
b6d265c
54d4664
ac8b6a2
a2a6314
37716b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 should still report a complaint:
I suspect the issue is returning
''
instead of the actual node text above.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.
bump on this 😇
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.
The highlighted line has two different properties (
#a
and#b
) and the example seems to be covered by this, am I missing something?typescript-eslint/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts
Lines 141 to 144 in 0afc516
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.
Ah sorry you're right, I missed that they're the same property. But this actually brings up that there's a bug in this PR for private identifiers: microsoft/TypeScript#42734 blocks them from being used.
https://deploy-preview-6028--typescript-eslint.netlify.app/play#ts=4.9.3&sourceType=module&code=MYGwhgzhAEBqCWB7cAXJA7aBvAUNaAxAA4BOiRApiSgJ7QC80ADANw57QoUQoAUAlNg75gidD2gQKIAGYNoAWTAoAFgDoSYdABNEAWwHQA-JxXwYALmgBXHRRnx0FbW2GTpMo2uJlK1Gmz4AL44IUA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WloHsalfkwCG8WmQAWo5uii9o-aJHBgAviHVA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA
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.
So do I revert it for now? What do you suggest?
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.
Well, it shouldn't throw an error - that's definitely a bug. But it also shouldn't complain. The rule should effectively ignore this case for now.
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.
Kept the private properties in computed nodes
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:
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.
@Josh-Cena @JoshuaKGoldberg @bradzacher WDYT?
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