Optional Chaining: Stage 1 plugin #545

xtuc · 2017-05-28T09:45:53Z

EDIT by hzoo: Check our spec-new label to follow newly implemented proposals!

Q	A
Bug fix?	n
Breaking change?	n
New feature?	y
Deprecations?	n
Spec compliancy?	y
Tests added/pass?	y/n
Fixed tickets	#328
License	MIT

Based on the spec here.

AST change:

interface MemberExpression <: Expression, Pattern {
  type: "MemberExpression";
  object: Expression | Super;
  property: Expression;
  computed: boolean;
+ optional: boolean | null;
}

interface CallExpression <: Expression {
  type: "CallExpression";
  callee: Expression | Super | Import;
  arguments: [ Expression | SpreadElement ];
+ optional: boolean | null;
}

interface NewExpression <: CallExpression {
  type: "NewExpression";
+ optional: boolean | null;
}

Edit:

TODO

Add test for a?.b.c.?.d? (skipping ?. in between)

…fined this will return undefined. If the object on which you want to access the property is defined, the value of the propery will be given back.

…ess. (.? or ?.[) If the object is undefined this will return undefined. If the object on which you want to access the property is defined, the value of the propery will be given back.

…at-optional-chaining

Jessidhia

Looks OK to me, but it already has merge conflicts 😕

AST change is adding an optional boolean to MemberExpression

Jessidhia · 2017-05-29T01:42:17Z

ast/spec.md

 }
 ```

-A member expression. If `computed` is `true`, the node corresponds to a computed (`a[b]`) member expression and `property` is an `Expression`. If `computed` is `false`, the node corresponds to a static (`a.b`) member expression and `property` is an `Identifier`.
+A member expression. If `computed` is `true`, the node corresponds to a computed (`a[b]`) member expression and `property` is an `Expression`. If `computed` is `false`, the node corresponds to a static (`a.b`) member expression and `property` is an `Identifier`. The `optional` flags indecates that the member expression can be called even if the object is null or undefined. If this is the object value (null/undefined) should be returned.


indicates

Jessidhia · 2017-05-29T01:46:58Z

src/tokenizer/index.js

@@ -391,6 +391,18 @@ export default class Tokenizer {
    return this.finishOp(code === 61 ? tt.eq : tt.prefix, 1);
  }

+  readToken_question() { // '?'
+    const next = this.input.charCodeAt(this.state.pos + 1);
+    if (next === 46) { // 46 = question '.'


Comment is confusing; just 46 = '.' should work.

Or just // '.' like the other parts of the code

hzoo · 2017-05-30T12:28:50Z

src/parser/expression.js

@@ -301,6 +301,24 @@ export default class ExpressionParser extends LValParser {
        node.object = base;
        node.callee = this.parseNoCallExpr();
        return this.parseSubscripts(this.finishNode(node, "BindExpression"), startPos, startLoc, noCalls);
+
+      } else if (this.eat(tt.questionDot)) {


Similar to the other PR, we need to add a flag. I added the steps in https://github.com/babel/babylon/blob/master/CONTRIBUTING.md#creating-a-new-plugin-spec-new.

hasPlugin('nullPropagation') or hasPlugin('optionalChaining'), etc. Probably optionalChaining since the repo was changed to that.

I added the plugin check, updated the corresponding documentation and also added an explicit error message since it's a new syntax.

hzoo · 2017-05-30T12:30:49Z

test/fixtures/experimental/optional-chaining/function-call/actual.js

@@ -0,0 +1 @@
+func?.()


This test doesn't have an expected?

Doesn't parse yet, need to be implemented (i'm still WIP)

hzoo · 2017-05-30T12:30:52Z

test/fixtures/experimental/optional-chaining/class-contructor-call/actual.js

@@ -0,0 +1 @@
+new C?.()


This test doesn't have an expected?

Doesn't parse yet, need to be implemented (i'm still WIP)

hzoo

Nice work!

add hasPlugin checks where necessary
add 2 missing expected checks (Optional Chaining Operator estree/estree#146, what do we do with new and function calls?)
Can we have some more tests? Like at least one to show that you can have a?.b?.c (multiple chaining in one member expression
Also that foo ? .3 : 0 is parsed as ternary and not optional chaining even while the hasPlugin check is true
Are there other early errors/tests? Like what if you try to do a.b?.c or a?.b.c.?.d, etc

cc @gisenberg for review, also @kristofdegrave, @rattrayalex

cpojer · 2017-05-30T13:51:24Z

When I experimented with this about three years ago, I also went the route to make MemberExpressions optional. Seems like that's the way to go! :)

kerhong · 2017-05-30T16:00:31Z

I believe this breaks valid syntax. The expression foo?.1:bar generates syntax error (Unexpected token), it should (and currently is) interpreted as foo ? 0.1 : bar.

hzoo · 2017-05-30T16:43:02Z

@kerhong Yep, I mentioned this in my review and it's also in https://github.com/tc39/proposal-optional-chaining#notes.

jridgewell · 2017-06-02T03:52:03Z

src/parser/expression.js

+            base.name === "async" &&
+            !this.canInsertSemicolon();
+
+          node.arguments = this.parseCallExpressionArguments(tt.parenR, possibleAsync);


There's no callee

codecov · 2017-06-03T05:20:13Z

Codecov Report

Merging #545 into master will decrease coverage by <.01%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
- Coverage   98.14%   98.14%   -0.01%     
==========================================
  Files          22       22              
  Lines        3620     3663      +43     
  Branches     1012     1023      +11     
==========================================
+ Hits         3553     3595      +42     
- Misses         24       25       +1     
  Partials       43       43

Flag	Coverage Δ
#babel	`79.66% <19.51%> (-0.79%)`	⬇️
#babylon	`96.83% <97.56%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
src/tokenizer/types.js	`100% <ø> (ø)`	⬆️
src/tokenizer/index.js	`98.44% <100%> (+0.02%)`	⬆️
src/parser/expression.js	`96.88% <96.77%> (-0.01%)`	⬇️
src/plugins/flow.js	`98.39% <0%> (ø)`	⬆️
src/plugins/estree.js	`99.19% <0%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324e2f0...4c8f4a2. Read the comment docs.

jridgewell · 2017-06-03T05:30:32Z

Opened a PR to complete the work, so hopefully this'll land soon.

Does anyone know why .? wasn't considered? As far as I can see, it fixes the foo?.3:0 edge case without introducing any others (there's a slight 1.?1:0 one, but that could be easily handled). I guess it really doesn't matter since you can't have a foo.3 anyways (numeric member expressions have to be computed).

Finish optionalChaining plugin

xtuc · 2017-06-03T14:09:50Z

Thanks for the PR @jridgewell, that's a nice job.

Does anyone know why .? wasn't considered?

Maybe because of the syntax? a.?b vs a?.b

jridgewell · 2017-06-03T19:30:07Z

It's actually brought up several times:

hzoo · 2017-06-05T01:55:02Z

cc @yungsters @zertosh if you want to review.

Will check again tomorrow

gisenberg

Looks good to me, great work! Thanks for your contribution!

hzoo · 2017-06-05T19:06:52Z

src/parser/expression.js

+        this.next();
+
+        const node = this.startNodeAt(startPos, startLoc);
+


Could probably just move node.optional = true; to here since it's in all the if conditionals?

I was trying to keep the property ordering consistent between optional and non-optional branches. See the #parseNew as well.

ljharb

Seems legit; I think the transform is where I'll have more opinions than the parsing :-)

ljharb · 2017-06-05T19:50:01Z

src/tokenizer/index.js

+  readToken_question() { // '?'
+    const next = this.input.charCodeAt(this.state.pos + 1);
+    const next2 = this.input.charCodeAt(this.state.pos + 2);
+    if (next === 46 && !(next2 >= 48 && next2 <= 57)) { // '.' not followed by a number


what are these magic numbers? could these be stored in named variables, so it's clear what "46" means?

These are char codes. I'll add some contants to improve readability, thanks.

davidyaha · 2017-06-05T21:15:02Z

🎉 🎉

hzoo · 2017-06-05T21:17:20Z

Nice work everyone! Really appreciating the reviews!

Babel transform PR is ~~babel/babel#5786 (please check that out next)~~

jridgewell · 2017-06-05T21:18:43Z

Actually, the transform PR is babel/babel#5813.

Kristof Degrave and others added 6 commits February 11, 2017 16:44

?. && ?[ as member access with nullPropagation. If the object is unde…

b902fe6

…fined this will return undefined. If the object on which you want to access the property is defined, the value of the propery will be given back.

#328 #205 ?. as nullPropagation. For now it only works for member acc…

1eaf016

…ess. (.? or ?.[) If the object is undefined this will return undefined. If the object on which you want to access the property is defined, the value of the propery will be given back.

feat: add optional MemberExpression

7b51979

Merge remote-tracking branch 'kristofdegrave/nullPropagation' into fe…

0927e24

…at-optional-chaining

feat: add tests

b2fdd94

fix: move tests in experimental

bc9edd1

xtuc added help wanted spec-new labels May 28, 2017

xtuc changed the title ~~Feat optional chaining~~ Null Propagation Operator May 28, 2017

xtuc added the Tag: New Feature label May 28, 2017

xtuc mentioned this pull request May 28, 2017

Null Propagation Operator babel/babel#5786

Closed

2 tasks

Jessidhia reviewed May 29, 2017

View reviewed changes

xtuc added 2 commits May 29, 2017 18:43

Merge branch 'master' into feat-optional-chaining

d3bc8fc

docs: fix typo in spec [skip ci]

03d89b6

hzoo reviewed May 30, 2017

View reviewed changes

hzoo suggested changes May 30, 2017

View reviewed changes

hzoo mentioned this pull request May 30, 2017

Optional Chaining: Stage 1 #328

Closed

hzoo changed the title ~~Null Propagation Operator~~ Optional Chaining: Stage 1 plugin May 30, 2017

xtuc added 2 commits May 30, 2017 18:35

feat: use syntax plugin

51bd87b

style: [skip ci] updated comment

26096d6

hzoo added the PR: WIP label May 30, 2017

xtuc and others added 4 commits May 30, 2017 20:12

feat: CallExpression support

9bcd85a

docs: NewExpression spec

b038600

fix: minor change

c1702e1

Merge branch 'master' into feat-optional-chaining

2dd624b

jridgewell reviewed Jun 2, 2017

View reviewed changes

Finish optionalChaining plugin

e1ec23c

Merge pull request #1 from jridgewell/pr/545

4628bb9

Finish optionalChaining plugin

hzoo requested a review from gisenberg June 3, 2017 14:10

jridgewell approved these changes Jun 3, 2017

View reviewed changes

hzoo mentioned this pull request Jun 5, 2017

Participation in developing the existential operator proposal Babel plugin facebook/idx#2

Closed

gisenberg approved these changes Jun 5, 2017

View reviewed changes

xtuc mentioned this pull request Jun 5, 2017

Optional Chaining Operator (Stage 1) babel/babel#5813

Merged

3 tasks

hzoo reviewed Jun 5, 2017

View reviewed changes

ljharb approved these changes Jun 5, 2017

View reviewed changes

add another test

4c8f4a2

hzoo approved these changes Jun 5, 2017

View reviewed changes

xtuc merged commit e064bb9 into babel:master Jun 5, 2017

xtuc deleted the feat-optional-chaining branch June 5, 2017 21:13

hzoo mentioned this pull request Jun 16, 2017

Optional Chaining Operator estree/estree#146

Closed

STRML mentioned this pull request Jun 27, 2017

Suggestion: "safe navigation operator", i.e. x?.y microsoft/TypeScript#16

Closed

vjpr mentioned this pull request Jul 3, 2017

Support optional chaining facebook/flow#4303

Closed

vjpr mentioned this pull request Jan 18, 2018

Tooling implementation status tc39/proposal-optional-chaining#44

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional Chaining: Stage 1 plugin #545

Optional Chaining: Stage 1 plugin #545

xtuc commented May 28, 2017 •

edited by hzoo

Loading

Jessidhia left a comment

Jessidhia May 29, 2017

Jessidhia May 29, 2017

hzoo May 30, 2017

hzoo May 30, 2017

xtuc May 30, 2017

hzoo May 30, 2017

xtuc May 30, 2017 •

edited

Loading

hzoo May 30, 2017

xtuc May 30, 2017 •

edited

Loading

hzoo left a comment •

edited

Loading

cpojer commented May 30, 2017

kerhong commented May 30, 2017

hzoo commented May 30, 2017

jridgewell Jun 2, 2017

codecov bot commented Jun 3, 2017 •

edited

Loading

jridgewell commented Jun 3, 2017 •

edited

Loading

xtuc commented Jun 3, 2017

jridgewell commented Jun 3, 2017

hzoo commented Jun 5, 2017 •

edited

Loading

gisenberg left a comment

hzoo Jun 5, 2017

jridgewell Jun 5, 2017

ljharb left a comment

ljharb Jun 5, 2017

xtuc Jun 5, 2017

davidyaha commented Jun 5, 2017

hzoo commented Jun 5, 2017 •

edited

Loading

jridgewell commented Jun 5, 2017

		this.next();

		const node = this.startNodeAt(startPos, startLoc);

		@@ -0,0 +1 @@
		func?.()

Optional Chaining: Stage 1 plugin #545

Optional Chaining: Stage 1 plugin #545

Conversation

xtuc commented May 28, 2017 • edited by hzoo Loading

TODO

Jessidhia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc May 30, 2017 • edited Loading

Choose a reason for hiding this comment

hzoo left a comment • edited Loading

Choose a reason for hiding this comment

cpojer commented May 30, 2017

kerhong commented May 30, 2017

hzoo commented May 30, 2017

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2017 • edited Loading

Codecov Report

jridgewell commented Jun 3, 2017 • edited Loading

xtuc commented Jun 3, 2017

jridgewell commented Jun 3, 2017

hzoo commented Jun 5, 2017 • edited Loading

gisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidyaha commented Jun 5, 2017

hzoo commented Jun 5, 2017 • edited Loading

jridgewell commented Jun 5, 2017

xtuc commented May 28, 2017 •

edited by hzoo

Loading

xtuc May 30, 2017 •

edited

Loading

xtuc May 30, 2017 •

edited

Loading

hzoo left a comment •

edited

Loading

codecov bot commented Jun 3, 2017 •

edited

Loading

jridgewell commented Jun 3, 2017 •

edited

Loading

hzoo commented Jun 5, 2017 •

edited

Loading

hzoo commented Jun 5, 2017 •

edited

Loading