Implement transform for nullish-coalescing operator #6483

azz · 2017-10-14T11:16:42Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	👍
Tests Added + Pass?	👍 (more needed)
Documentation PR
Any Dependency Changes?	👍 depends on babylon beta.29

Adds the babel transform for the nullish-coalescing operator proposal, currently at stage 1.

Adds two new packages:

babel-plugin-syntax-nullish-coalescing-operator
Just turns on the "nullishCoalescingOperator" plugin.

babel-plugin-transform-nullish-coalescing-operator
Transforms:

var x = obj.foo ?? "bar";

Into:

var _ref;
var x = (_ref = obj.foo, _ref != null ? _ref : "bar");

Adds transform-nullish-coalescing-operator to babel-preset-stage-1.

A couple of tests are added but more are needed. Please comment with any tests cases you can think of!

Babylon PR: babel/babylon#761
Ref: babel/proposals#14
Proposal: https://github.com/gisenberg/proposal-null-coalescing
Spec Text: https://littledan.github.io/proposal-nullary-coalescing/

jridgewell · 2017-10-15T22:03:00Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+          return;
+        }
+
+        const scope = path.scope.parent || path.scope;


Just path.scope.

I did this to fix an issue with function default arguments.

function foo(a, b = a ?? 0) {}

Would otherwise transform to

function foo(a, b = (_ref = a, ref != null ? _ref : 0)) { var _ref; }

Which leaks a global. Is there a better way to handle this?

jridgewell · 2017-10-15T22:05:35Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+        }
+
+        const scope = path.scope.parent || path.scope;
+        const ref = scope.generateUidIdentifier("ref");


We can use #generateUidIdentifierBasedOnNode, which'll create a better identifier name.

jridgewell · 2017-10-15T22:05:45Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+            t.assignmentExpression("=", ref, node.left),
+            t.conditionalExpression(
+              t.binaryExpression("!=", ref, t.nullLiteral()),
+              ref,


Need to clone this node.

jridgewell · 2017-10-15T22:10:40Z

packages/babel-types/src/constants.js

@@ -38,6 +38,7 @@ export const NUMBER_BINARY_OPERATORS = [
 ];
 export const BINARY_OPERATORS = [
  "+",
+  "??",


This belongs in LOGICAL_OPERATORS, not BINARY_OPERATORS

Ah, forgot to move it. Thanks.

jridgewell · 2017-10-15T22:15:59Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+    visitor: {
+      LogicalExpression(path) {
+        const { node } = path;
+        if (node.operator !== OPERATOR) {


Nit: Just inline it.

babel-bot · 2017-10-17T02:57:34Z

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

bakkot · 2017-10-17T04:52:27Z

packages/babel-plugin-transform-nullish-coalescing-operator/README.md

+
+```javascript
+var _ref;
+var foo = (_ref = object.foo, _ref != null ? _ref : "default");


Depending on how concerned we are about being exactly right... this isn't. document.all ?? "default" should give you document.all, not "default", even though document.all is == null.

(Not familiar with the HTML Document Dot All object? document.all is a value which is == null and == void 0 but not === either; it has typeof document.all === 'undefined' but can be called without throwing. Why? Historical reasons. Welcome to the web platform, where everything is terrible.)

If we care about this, it should instead be ref !== null && ref !== void 0 (or however babel outputs undefined). Alternatively, we could choose to not care. Basically no modern code has any legitimate reason to interact with document.all anyway.

I think we should maybe have two options here; the default, spec, should indeed do ref === null && ref === undefined, but loose could do == null if there's a performance impact.

It is plausible that the former is faster, actually, depending on the engine. x != null has to end up the same as x !== null && x !== undefined && x !== document.all, which is more expensive than x !== null && x !== undefined.

Can we just forget that document.all exists? 😉

There was actual confusion at the TC39 meeting about whether document.all is included, so it'd be nice if Babel got this right, at least in spec mode. It would be good to get strong user feedback here.

Get it right, as in pretend it doesn't exist? That's my preferred solution. Do we have any other syntax that explicitly accounts for document.all?

_ref !== null && _ref !== undefined would account for it properly.

Changed to ref !== null && _ref !== void 0 for compliance.

bakkot

LGTM otherwise.

ljharb · 2017-10-17T05:03:33Z

.gitignore

@@ -1,4 +1,5 @@
 .DS_Store
+.vscode


(not a big deal, but this seems like the sort of thing to put in your global gitconfig, rather than adding specific editor knowledge to individual repos)

ljharb · 2017-10-17T05:04:12Z

packages/babel-plugin-syntax-nullish-coalescing-operator/package.json

@@ -0,0 +1,13 @@
+{
+  "name": "babel-plugin-syntax-nullish-coalescing-operator",
+  "version": "7.0.0-beta.3",


any reason this can't just start at 1.0.0? I thought babel 7 was moving away from pegged version numbers.

Just following precedent. @hzoo should this be changed?

Yeah we need to move all proposals/stage-x presets into the new experimental folder and then version those separately (separate pr)

ljharb · 2017-10-17T05:04:30Z

packages/babel-plugin-syntax-nullish-coalescing-operator/src/index.js

@@ -0,0 +1,7 @@
+export default function() {


should this be an arrow, since it doesn't need this?

function is ok, since we use it in every other plugin.

I'd argue for it to be an arrow because it's not supposed to be constructible -- but since all other plugins are written like this, changing it would be outside the scope of this PR. Best to keep to the pattern and then later we can codemod everything to arrows if we decide to.

ljharb · 2017-10-17T05:05:02Z

packages/babel-plugin-transform-nullish-coalescing-operator/package.json

@@ -0,0 +1,17 @@
+{
+  "name": "babel-plugin-transform-nullish-coalescing-operator",
+  "version": "7.0.0-beta.3",


same question here

ljharb · 2017-10-17T05:05:09Z

packages/babel-plugin-transform-nullish-coalescing-operator/package.json

+  "description": "Remove nullish coalescing operator",
+  "repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-nullish-coalescing-opearator",
+  "license": "MIT",
+  "main": "lib/index.js",


could this just be lib?

Yeah. I just copied it from a similar package.

xtuc · 2017-10-17T06:33:18Z

We should probably wait for #6495 before merging and renaming it in the mean time.

xtuc

I didn't realize it was your first contribution @azz. Nice job 👍

hzoo · 2017-10-17T12:56:32Z

Ah yeah I merged the scoped packages rename so will need to make that change too 😛 and rebase

jridgewell · 2017-10-17T18:47:57Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+          return;
+        }
+
+        const scope = path.scope.parent || path.scope;


Still needs to be just path.scope. The default parameter memoization affects other transforms, and we'll need to figure out a way to deal with them all together.

jridgewell · 2017-10-17T18:48:36Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+          t.sequenceExpression([
+            t.assignmentExpression("=", ref, node.left),
+            t.conditionalExpression(
+              t.binaryExpression("!=", ref, t.nullLiteral()),


ref needs to be cloned here.

What's the rule for when a node needs to be cloned? When it is passed to multiple builders?

Whenever it's reusued in multiple places in an AST. We're just findings bugs with reused nodes, so they're still a bit hard to spot.

- Use generateUidIdentifierBasedOnNode - Inline "??" - Clone ref node - Move "??" to LogicalExpression in babel-types

ljharb · 2017-10-18T00:50:26Z

packages/babel-plugin-transform-nullish-coalescing-operator/README.md

+
+```javascript
+var _ref;
+var foo = (_ref = object.foo, _ref != null ? _ref : "default");


to clarify; i think that using != null by default should be a blocker, since it's not in compliance with the spec

ljharb · 2017-10-18T00:50:44Z

packages/babel-plugin-transform-nullish-coalescing-operator/package.json

+  "description": "Remove nullish coalescing operator",
+  "repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-nullish-coalescing-opearator",
+  "license": "MIT",
+  "main": "lib/index.js",


ljharb · 2017-10-18T00:50:55Z

.gitignore

@@ -1,4 +1,5 @@
 .DS_Store
+.vscode


azz · 2017-10-18T01:54:30Z

Happy to go either way with document.all. Do we change the transform or change the proposal?

It will be difficult to write a test for it either way without exposing natives syntax in v8.

assert.equal(%GetUndetectable() ?? "foo", %GetUndetectable());

bakkot · 2017-10-18T02:36:37Z

If I recall correctly, we explicitly discussed document.all in committee and decided it was not "nullish", for the purposes of this proposal. So I think the right thing to do is change the transform.

I wouldn't worry too much about a test for document.all, personally, though other people might disagree. Maybe a comment in the transform (and/or readme?) explaining why you aren't using != null.

ljharb · 2017-10-18T03:59:24Z

An inline comment is probably most useful.

ljharb

Thanks, LGTM!

Jessidhia

LGTM

I kinda want to add a test for document.all if typeof document !== 'undefined' but I don't want browsers to feel like they can't remove document.all because they would break babel's tests ;)

(I don't even know if we can execute our tests in a browser)

Jessidhia · 2017-10-18T04:41:13Z

packages/babel-plugin-syntax-nullish-coalescing-operator/src/index.js

@@ -0,0 +1,7 @@
+export default function() {


I'd argue for it to be an arrow because it's not supposed to be constructible -- but since all other plugins are written like this, changing it would be outside the scope of this PR. Best to keep to the pattern and then later we can codemod everything to arrows if we decide to.

Jessidhia · 2017-10-18T04:58:59Z

packages/babel-plugin-transform-nullish-coalescing-operator/README.md

+```
+
+> **NOTE:** We cannot use `!= null` here because `document.all` is `!= null` and
+> `document.all` has been deemed not "nullish".


This also has a runtime performance benefit, even if it happens to be slightly bigger. See nodejs/node#15178 for example.

Jessidhia · 2017-10-18T05:02:33Z

packages/babel-plugin-transform-nullish-coalescing-operator/src/index.js

+        }
+
+        const ref = scope.generateUidIdentifierBasedOnNode(node.left);
+        scope.push({ id: ref });


A possible future optimization is to try to reuse the ref if they are in the same scope and the ref does not escape.

I don't know if the scope API supports escape analysis, though.

bakkot · 2017-10-18T05:17:52Z

packages/babel-plugin-transform-nullish-coalescing-operator/README.md

+var foo = (_object$foo = object.foo, _object$foo !== null && _object$foo !== void 0 ? _object$foo : "default");
+```
+
+> **NOTE:** We cannot use `!= null` here because `document.all` is `!= null` and


Nit: 'document.all is != null' should be 'document.all is == null'

Fixed, thanks.

jridgewell

We don't have a way to test in browsers currently, but this is fine. Let's make a "good first issue" to transform into != null in loose mode.

jridgewell · 2017-10-18T07:06:34Z

...ullish-coalescing-operator/test/fixtures/nullish-coalescing/transform-in-default/expected.js

@@ -0,0 +1,3 @@
+function foo(foo, bar = (_foo = foo, _foo !== null && _foo !== void 0 ? _foo : "bar")) {
+  var _foo;


This is definitely incorrect, but we can't do better at the moment. It'll take a try-finally to get right, which is kinda sad.

var _foo; function foo(foo, bar = (_foo = foo, _foo !== null && _foo !== void 0 ? _foo : "bar")) { try { // function body } finally { _foo = void 0; } }

It's probably better to use an IIFE, no?

function foo(foo, bar = function(){let _foo = foo; return _foo !== null && _foo !== void 0 ? _foo : "bar";}()) {}

Avoids leaking anything to the outer scope, if that's the concern.

😄 Oh man, I totally overlooked that this value can be anything, including a IIFE.

Kids, don't try this at home!

jridgewell · 2017-10-18T07:16:34Z

Oh, and we need to update optional chaining to match the document.all semantics.

azz mentioned this pull request Oct 15, 2017

Nullish Coalescing (Stage 3) babel/proposals#14

Closed

2 tasks

jridgewell requested changes Oct 15, 2017

View reviewed changes

hzoo added the Spec: Nullish Coalescing Operator label Oct 15, 2017

azz force-pushed the nullish-coalescing-operator branch 2 times, most recently from 21dccff to fae540f Compare October 17, 2017 02:49

hzoo requested review from littledan, bakkot, gisenberg and ljharb October 17, 2017 03:55

bakkot reviewed Oct 17, 2017

View reviewed changes

bakkot approved these changes Oct 17, 2017

View reviewed changes

ljharb reviewed Oct 17, 2017

View reviewed changes

xtuc approved these changes Oct 17, 2017

View reviewed changes

hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Oct 17, 2017

jridgewell reviewed Oct 17, 2017

View reviewed changes

azz added 5 commits October 18, 2017 09:49

Implement transform for nullish-coalescing operator

cbf2663

Update example output

6f6764a

Switch from BinaryExpression to LogicalExpression

a82c7b0

Address review comments

db4026f

- Use generateUidIdentifierBasedOnNode - Inline "??" - Clone ref node - Move "??" to LogicalExpression in babel-types

Fix reference to @babel/helper-plugin-test-runner

dea0733

azz force-pushed the nullish-coalescing-operator branch from fae540f to dea0733 Compare October 17, 2017 23:05

azz added 2 commits October 18, 2017 10:29

Fix reference to @babel/plugin-syntax-nullish-coalescing-operator

cf042ac

Don't use parent scope

6fe1bef

jridgewell approved these changes Oct 18, 2017

View reviewed changes

ljharb requested changes Oct 18, 2017

View reviewed changes

Remove .vscode from .gitignore, change 'lib/index.js' to 'lib'

3c18fd9

Ensure document.all ?? 0 === document.all

383b470

ljharb approved these changes Oct 18, 2017

View reviewed changes

Jessidhia approved these changes Oct 18, 2017

View reviewed changes

bakkot reviewed Oct 18, 2017

View reviewed changes

Fix note and copy to an inline comment

f48410e

jridgewell approved these changes Oct 18, 2017

View reviewed changes

jridgewell merged commit 99be60b into babel:master Oct 18, 2017

azz deleted the nullish-coalescing-operator branch October 18, 2017 23:12

jridgewell mentioned this pull request Jul 24, 2019

Implementation status tracking tc39/proposal-nullish-coalescing#47

Closed

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

		@@ -0,0 +1,3 @@
		function foo(foo, bar = (_foo = foo, _foo !== null && _foo !== void 0 ? _foo : "bar")) {
		var _foo;

Implement transform for nullish-coalescing operator #6483

Implement transform for nullish-coalescing operator #6483

Conversation

azz commented Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

azz Oct 15, 2017 • edited Loading

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

babel-bot commented Oct 17, 2017 • edited Loading

bakkot Oct 17, 2017 • edited Loading

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

Choose a reason for hiding this comment

bakkot 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

hzoo Oct 17, 2017 • edited Loading

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

Choose a reason for hiding this comment

xtuc commented Oct 17, 2017

xtuc left a comment

Choose a reason for hiding this comment

hzoo commented Oct 17, 2017

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

azz commented Oct 18, 2017

bakkot commented Oct 18, 2017

ljharb commented Oct 18, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

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

jridgewell 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

jridgewell commented Oct 18, 2017

azz commented Oct 14, 2017 •

edited

Loading

azz Oct 15, 2017 •

edited

Loading

babel-bot commented Oct 17, 2017 •

edited

Loading

bakkot Oct 17, 2017 •

edited

Loading

hzoo Oct 17, 2017 •

edited

Loading

ljharb commented Oct 18, 2017 •

edited

Loading