fix(typescript-estree): handle opt computed method with MemberExpression #1100
Conversation
|
Thanks for the PR, @sosukesuzuki! 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 Report
@@ Coverage Diff @@
## master #1100 +/- ##
=======================================
Coverage 94.02% 94.02%
=======================================
Files 115 115
Lines 5123 5123
Branches 1434 1434
=======================================
Hits 4817 4817
Misses 177 177
Partials 129 129
|
| @@ -1044,7 +1044,8 @@ export class Converter { | |||
| } | |||
|
|
|||
| if ( | |||
| result.key.type === AST_NODE_TYPES.Identifier && | |||
| (result.key.type === AST_NODE_TYPES.Identifier || | |||
| result.key.type === AST_NODE_TYPES.MemberExpression) && | |||
| node.questionToken | |||
| ) { | |||
| result.key.optional = true; | |||
There was a problem hiding this comment.
Why add optional: true to key? Let's add it to the property node like Babel does. Or better, for backward compatibility, to both.
There was a problem hiding this comment.
Also the expression does not necessarily have to be a member expression. Syntactically, TS allows any expression there, it only puts some limitations on its type (which should not concern us here). E.g. check out this valid (albeit useless) TypeScript code:
declare function f(): string;
class Foo {
[f()]?() {
return 1;
}
}There was a problem hiding this comment.
@thorn0 (/cc @bradzacher ) I also wondered about that. However, optional does not exist in MethodDefinitionBase. (https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/ts-estree/ts-estree.ts#L510-L519). Can we update it?
There was a problem hiding this comment.
Hmm..
Tracing it back. This pattern goes back a long way.
Originally introduced in 2017 eslint/typescript-eslint-parser#187
Why did it get put there instead of on the property itself? I don't know.
Digging into this more - we don't do a good job of standardised support for optional members.
I'd be happy if you standardised things, without doing a breaking change.
We can file an issue for the 3.0.0 release to remove the old weird cases.
const a = 'aa';
const b = 'bb';
const c = 'cc';
const d = 'dd';
const e = 'ee';
const f = 'ff';
abstract class Foo {
a?: string; // ClassProperty is marked optional
[a]?: string; // nothing is marked optional, should be ClassProperty
['aaa']?: string; // nothing is marked optional, should be ClassProperty
b?() {} // Identifier is marked optional, should be MethodDefinition
[b]?() {} // Identifier is marked optional, should be MethodDefinition
['bbb']?() {} // nothing is marked optional, should be MethodDefinition
c?(): void // Identifier is marked optional, should be MethodDefinition
[c]?(): void // Identifier is marked optional, should be MethodDefinition
['ccc']?(): void // nothing is marked optional, should be MethodDefinition
abstract d?: string; // TSAbstractClassProperty is marked optional
abstract [d]?: string; // nothing is marked optional, should be TSAbstractClassProperty
abstract ['ddd']?: string; // TSAbstractClassProperty is marked optional
abstract e?(): void // Identifier is marked optional, should be TSAbstractMethodDefinition
abstract [e]?(): void // Identifier is marked optional, should be TSAbstractMethodDefinition
abstract ['eee']?(): void // nothing is marked optional, should be TSAbstractMethodDefinition
constructor(
private f?: string, // Identifier is marked optional, should be that AS WELL AS TSParameterProperty
) { }
}
interface Bar {
a?: string; // TSPropertySignature is marked optional
[a]?: string; // TSPropertySignature is marked optional
['aaa']?: string; // TSPropertySignature is marked optional
b?(): void // TSMethodSignature is marked optional
[b]?(): void // TSMethodSignature is marked optional
['bbb']?(): void // TSMethodSignature is marked optional
}
type Baz1 = {
[k in string]?: string // TSMappedType is marked optional
}
type Baz2 = {
[k in string]+?: string // TSMappedType is marked optional = "+"
}
type Baz3 = {
[k in string]-?: string // TSMappedType is marked optional = "-"
}
function Buzz(
a?: string, // Identifier is marked optional
): void {}astexplorer repl
ts playground repl
LMK if this makes sense
There was a problem hiding this comment.
Thanks, @bradzacher!! ( /cc @thorn0 )
Personally, I think that it is better to keep adding optional to key until releasing 3.0.
Because, in order for us to add optional to properties, we need to update some basic node type definitions(e.g. MethodDefinitionBase ). I am reluctant to do it with minor version upgrades ...
There was a problem hiding this comment.
We can file an issue for the 3.0.0 release to remove the old weird cases.
👍
There was a problem hiding this comment.
in order for us to add optional to properties, we need to update some basic node type definitions
It's totally okay as long as the property you add is optional.
"Standardising things without doing a breaking change" means:
- in cases where
optionalis added to thekeyin the current version, add it to both the key and the property/method - in other cases, add it only to the property/method
Fix #1097