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
[ts] support optional chain call with generic #13513
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47914/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1301462:
|
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.
Thanks! Could you add also a test for:
f
?.<Q>()
@fedeci I patched it! |
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.
whether we need to check the noCalls state like this one.
Yes we should check noCalls
when consuming ?.
. e.g.
new f?.<string>()
should throw because optional chain can not include new expression.
I have no idea on failed linting test ..:'( |
packages/babel-parser/test/fixtures/typescript/type-arguments/call-optional-chain/output.json
Outdated
Show resolved
Hide resolved
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 generated AST looks correct, but I think that there is a bug in the transform plugin:
This code should keep the ?.
and just strip away the <Q>
part.
"type": "ChainExpression", | ||
"start":15,"end":30,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":15}}, | ||
"expression": { | ||
"type": "CallExpression", |
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 be an OptionalCallExpression
.
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.
Ok, I'm gonna fix it. but there is optional: true
at the end of this depth. Is it different? 🤔
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.
Yes! optional: true
means "here you have a question mark", while OptionalCallExpression
means "there is a question mark somewhere".
For example, in a?.()()
both are OptionalCallExpression
but only the first one has optional: true
.
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 behaviour is correct. The test enables the estree
plugin, which converts OptionalCallExpression
to a ChainExpression
.
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 I didn't realize it.
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.
Thanks a lot! I patched the code, and other part is now OptionalCallExpression
.
@@ -2111,7 +2124,14 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
// $FlowIgnore | |||
node.optional = false; | |||
} | |||
return this.finishCallExpression(node, state.optionalChainMember); | |||
if (isOptionalCall) { | |||
node.optional = true; |
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.
When state.optionalChainMember
is set, we can merge this branch with the preceding one.
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 couldn't understand the above code. I don't know the reason when state.optionalChainMember
is true, then node.optional
would be false.
babel/packages/babel-parser/src/plugins/typescript/index.js
Lines 2127 to 2130 in b3990fa
if (state.optionalChainMember) { | |
// $FlowIgnore | |
node.optional = false; | |
} |
You mean like this?
if (state.optionalChainMember) {
node.optional = isOptional? true: false;
}
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.
Yes. The isOptional
should be assigned to node.optional
.
@@ -2127,6 +2147,10 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
this.unexpected(); | |||
}); | |||
|
|||
if (!result && isOptionalCall) { | |||
this.unexpected(this.state.pos, tt.parenL); |
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 think we can throw at better position: We can track the position after typeArguments
is parsed but we do not see a left parenthesis token. Then we throw on this position that we are expecting a left paren after type arguments.
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.
If we throw an error on this condition, the result
would be undefined
by tsTryParseAndCatch
, and also the error message would be ignored.
I found that the error position is wrong (it should be "Unexpected token, expected "(" (1:12)" not (1:4)) because the error is restored by tsTryParseAndCatch
.
I wanna throw an error with the accurate error message at a better position.
To handle this, we can make unexpectedError
in packages/babel-parser/src/parser/util.js
and return this.unexpectedError(this.state.pos, tt.parenL)
on that condition., then change this part like this.
if (result?.failState) throw result.node;
But I'm not sure that we could use node
to handle the error. It seems weird. I need help 😢
"plugins": [ | ||
"typescript" | ||
], | ||
"throws": "Unexpected token, expected \"(\" (1:4)" |
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 position is wrong. This should be Unexpected token, expected "(" (1:12)
.
889a19a
to
dac5990
Compare
I patched it :) |
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 left two minor comments, but otherwise it looks good 👍
this.match(tt.questionDot) && | ||
this.lookaheadCharCode() === charCodes.lessThan | ||
) { | ||
state.optionalChainMember = isOptionalCall = true; |
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.
Nit: we can move this after the if (noCalls) {}
check, since we don't need to set if if we stop and return.
@@ -2095,6 +2109,11 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
const typeArguments = this.tsParseTypeArguments(); | |||
|
|||
if (typeArguments) { | |||
if (isOptionalCall && !this.match(tt.parenL)) { | |||
error = [this.state.pos, tt.parenL]; |
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.
We only need to store the error position (in a missingParenErrorPos
variable), and then later do
if (missingParenErrorPos) {
this.unexpected(missingParenErrorPos, tt.parenL);
}
@JLHwung @fedeci @nicolo-ribaudo |
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.
Nice!
7952a1f
to
1301462
Compare
I referenced this part. I'm little confused whether we need to check the
noCalls
state like this one.babel/packages/babel-parser/src/plugins/flow/index.js
Lines 3098 to 3104 in 90d5eb7