Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Add more tests for the nullish coalescing operator #762

Merged
merged 3 commits into from
Oct 16, 2017

Conversation

azz
Copy link
Member

@azz azz commented Oct 15, 2017

Q A
Bug fix? no
Breaking change? no (breaks beta 28)
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #761 (comment)
License MIT

Added some more tests for associativity, multiline parsing, and precedence.

Due to this operator being similar to ||, and it being in the LogicalORExpression non-terminal in the grammar, should it move from BinaryExpression to the LogicalExpression AST node?

Update:

?? has been moved to LogicalExpression

@hzoo
Copy link
Member

hzoo commented Oct 15, 2017

Oh yeah that may be better actually

@azz
Copy link
Member Author

azz commented Oct 15, 2017

Has been changed from Binary- to LogicalExpression in this PR.

@hzoo hzoo requested a review from jridgewell October 15, 2017 22:15
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests for || especially:

a || b ?? c;
a ?? b || c;
a && b ?? c;
a ?? b && c;

@hzoo hzoo requested review from littledan and gisenberg October 16, 2017 12:59
@jridgewell
Copy link
Member

So:

a || b ?? c === (a || b) ?? c
a ?? b || c === (a ?? b) || c

But:

a && b ?? c === (a && b) ?? c
a ?? b && c === a ?? (b && c)

Do we have any data for why this was chosen?

@littledan
Copy link

@jridgewell Is that what this code does? In the spec I wrote for ??, it parses exactly like ||, and in particular && groups more tightly.

@jridgewell
Copy link
Member

Is that what this code does?

Yes.

In the spec I wrote for ??, it parses exactly like ||, and in particular && groups more tightly.

That appears to be what's happening? I was just curious why it's not tighter than && or looser than ||.

@littledan
Copy link

@jridgewell I thought it would make sense to be exactly the same because people are likely to be upgrading their || to ??, and why change anything? If I were to change it, it would be to be looser than ||. Seems like this deserves a bug to discuss further.

@azz
Copy link
Member Author

azz commented Oct 16, 2017

My intuition is it should be completely hot-swappable with ||. Great for codemods/fixers.

@littledan
Copy link

Opened tc39/proposal-nullish-coalescing#15 to discuss this question; for now, the spec is hot-swappable like that.

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We can discuss the precedence in tc39/proposal-nullish-coalescing#15.

@hzoo hzoo merged commit eaf054b into babel:master Oct 16, 2017
@azz azz deleted the nullish-tests branch October 17, 2017 01:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants