[ts] support optional chain call with generic by lala7573 · Pull Request #13513 · babel/babel

lala7573 · 2021-06-27T09:49:09Z

Q	A
Fixed Issues?	Fixes #13400
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	👍
Documentation PR Link
Any Dependency Changes?
License	MIT

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

    
           if (this.match(tt.questionDot) && this.isLookaheadToken_lt()) { 
        
             subscriptState.optionalChainMember = true; 
        
             if (noCalls) { 
        
               subscriptState.stop = true; 
        
               return base; 
        
             } 
        
             this.next();

babel-bot · 2021-06-27T09:51:53Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47914/

codesandbox-ci · 2021-06-27T09:53:17Z

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:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

fedeci

Thanks! Could you add also a test for:

f
?.<Q>()

lala7573 · 2021-06-28T01:15:41Z

@fedeci I patched it!

packages/babel-parser/src/plugins/typescript/index.js

JLHwung

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.

lala7573 · 2021-06-30T13:58:00Z

I have no idea on failed linting test ..:'(

packages/babel-parser/test/fixtures/typescript/type-arguments/call-optional-chain/output.json

nicolo-ribaudo

The generated AST looks correct, but I think that there is a bug in the transform plugin:

https://babeljs.io/repl/build/47157/#?browsers=chrome%2090&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=GYfgdAPAigfAFASgNxA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Ctypescript&prettier=false&targets=&version=7.14.7%2Bpr.13513&externalPlugins=

This code should keep the ?. and just strip away the <Q> part.

nicolo-ribaudo · 2021-07-02T08:39:40Z

packages/babel-parser/test/fixtures/estree/typescript/optional-chaining/output.json

+          "type": "ChainExpression",
+          "start":15,"end":30,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":15}},
+          "expression": {
+            "type": "CallExpression",


This should be an OptionalCallExpression.

Ok, I'm gonna fix it. but there is optional: true at the end of this depth. Is it different? 🤔

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.

The behaviour is correct. The test enables the estree plugin, which converts OptionalCallExpression to a ChainExpression.

Oh I didn't realize it.

Thanks a lot! I patched the code, and other part is now OptionalCallExpression.

packages/babel-parser/src/plugins/typescript/index.js

JLHwung · 2021-07-02T18:14:36Z

packages/babel-parser/src/plugins/typescript/index.js

              }
-              return this.finishCallExpression(node, state.optionalChainMember);
+              if (isOptionalCall) {
+                node.optional = true;


When state.optionalChainMember is set, we can merge this branch with the preceding one.

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; }

Yes. The isOptional should be assigned to node.optional.

JLHwung · 2021-07-02T18:19:27Z

packages/babel-parser/src/plugins/typescript/index.js

        });

+        if (!result && isOptionalCall) {
+          this.unexpected(this.state.pos, tt.parenL);


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.

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 😢~~

lala7573 · 2021-07-03T16:20:21Z

...abel-parser/test/fixtures/typescript/type-arguments/call-optional-chain-invalid/options.json

+  "plugins": [
+    "typescript"
+  ],
+  "throws": "Unexpected token, expected \"(\" (1:4)"


The position is wrong. This should be Unexpected token, expected "(" (1:12).

lala7573 · 2021-07-07T01:36:09Z

I patched it :)

nicolo-ribaudo

I left two minor comments, but otherwise it looks good 👍

nicolo-ribaudo · 2021-07-07T15:42:31Z

packages/babel-parser/src/plugins/typescript/index.js

+        this.match(tt.questionDot) &&
+        this.lookaheadCharCode() === charCodes.lessThan
+      ) {
+        state.optionalChainMember = isOptionalCall = true;


Nit: we can move this after the if (noCalls) {} check, since we don't need to set if if we stop and return.

nicolo-ribaudo · 2021-07-07T15:44:22Z

packages/babel-parser/src/plugins/typescript/index.js


          if (typeArguments) {
+            if (isOptionalCall && !this.match(tt.parenL)) {
+              error = [this.state.pos, tt.parenL];


We only need to store the error position (in a missingParenErrorPos variable), and then later do

if (missingParenErrorPos) { this.unexpected(missingParenErrorPos, tt.parenL); }

lala7573 · 2021-07-22T02:10:41Z

@JLHwung @fedeci @nicolo-ribaudo
Is there anything to merge this? 🙏

fedeci

Nice!

The Lint error is related.

fedeci added area: typescript pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jun 27, 2021

fedeci approved these changes Jun 27, 2021

View reviewed changes

JLHwung reviewed Jun 28, 2021

View reviewed changes

packages/babel-parser/src/plugins/typescript/index.js Show resolved Hide resolved

JLHwung reviewed Jun 28, 2021

View reviewed changes

lala7573 requested review from JLHwung, fedeci and nicolo-ribaudo June 30, 2021 07:04

nicolo-ribaudo reviewed Jun 30, 2021

View reviewed changes

packages/babel-parser/test/fixtures/typescript/type-arguments/call-optional-chain/output.json Outdated Show resolved Hide resolved

lala7573 requested a review from nicolo-ribaudo July 1, 2021 02:21

lala7573 force-pushed the fix-13400 branch from 7935bf3 to 76abdb1 Compare July 1, 2021 12:06

nicolo-ribaudo reviewed Jul 1, 2021

View reviewed changes

lala7573 force-pushed the fix-13400 branch from cbfe166 to adedffd Compare July 2, 2021 02:27

nicolo-ribaudo reviewed Jul 2, 2021

View reviewed changes

JLHwung reviewed Jul 2, 2021

View reviewed changes

packages/babel-parser/src/plugins/typescript/index.js Outdated Show resolved Hide resolved

JLHwung reviewed Jul 2, 2021

View reviewed changes

lala7573 commented Jul 3, 2021

View reviewed changes

lala7573 force-pushed the fix-13400 branch 2 times, most recently from 889a19a to dac5990 Compare July 7, 2021 01:34

lala7573 requested review from JLHwung and nicolo-ribaudo July 7, 2021 01:35

nicolo-ribaudo approved these changes Jul 7, 2021

View reviewed changes

fedeci approved these changes Jul 22, 2021

View reviewed changes

JLHwung previously approved these changes Jul 23, 2021

View reviewed changes

lala7573 and others added 8 commits August 4, 2021 16:58

support optional chain call with generic

c0a3066

Add testcase handling new line

c61421a

allow only call if optional chain type parameter

ef70f68

use exact error message

b620ec1

add transform-typescript optional-chain call test

2cb08cb

use exact error message

926f862

apply feedbacks

2941d60

Fix lint error

1301462

nicolo-ribaudo force-pushed the fix-13400 branch from 7952a1f to 1301462 Compare August 4, 2021 15:03

nicolo-ribaudo merged commit 0671afc into babel:main Aug 4, 2021

nicolo-ribaudo changed the title ~~support optional chain call with generic~~ [ts] support optional chain call with generic Aug 4, 2021

This was referenced Aug 5, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.14 to 7.15.0 filecoin-project/slate#863

Merged

chore(deps-dev): bump @babel/core from 7.13.14 to 7.15.0 filecoin-project/slate#862

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 4, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2021

	if (this.match(tt.questionDot) && this.isLookaheadToken_lt()) {
	subscriptState.optionalChainMember = true;
	if (noCalls) {
	subscriptState.stop = true;
	return base;
	}
	this.next();

	if (state.optionalChainMember) {
	// $FlowIgnore
	node.optional = false;
	}

Uh oh!

Conversation

lala7573 commented Jun 27, 2021 • edited by gitpod-io bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fedeci left a comment

Choose a reason for hiding this comment

Uh oh!

lala7573 commented Jun 28, 2021

Uh oh!

Uh oh!

JLHwung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 commented Jun 30, 2021

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 commented Jul 7, 2021

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lala7573 commented Jul 22, 2021

Uh oh!

fedeci left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lala7573 commented Jun 27, 2021 •

edited by gitpod-io bot

Loading

babel-bot commented Jun 27, 2021 •

edited

Loading

codesandbox-ci bot commented Jun 27, 2021 •

edited

Loading

JLHwung left a comment •

edited

Loading

lala7573 Jul 2, 2021 •

edited

Loading

lala7573 Jul 3, 2021 •

edited

Loading

lala7573 Jul 3, 2021 •

edited

Loading