Added babel-helper-split-export-declaration #7313

Andarist · 2018-02-02T10:30:02Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	x
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?	internal
License	MIT

Extracted from - #6963

cc @nicolo-ribaudo

Andarist · 2018-02-02T10:31:24Z

packages/babel-helper-split-export-declaration/src/index.js

+      ? t.exportNamedDeclaration(null, [
+          t.exportSpecifier(t.cloneNode(id), t.identifier("default")),
+        ])
+      : t.exportDefaultDeclaration(t.cloneNode(id));


@loganfsmyth commented on this:

Even if it doesn't have an ID, cyclic dependencies can still rely on it being hoisted. It's never safe to do export default foo; in this case.

I'm not sure when that could have an impact? is it about smth like this?

export default (() => console.log('side effect'))()

It might be about something like rollup/rollup#1784

// index.js import "./a"; export default function(){ return 4; } // a.js import getFour from "./index"; getFour();

ok, from what I understand this can affect anonymous functions and classes, right? import/exports are hoisted so with the given example we end up with

import "./a"; function _default() { return 4; } export default _default;

which is later transformed to by cjs transform

"use strict"; exports.__esModule = true; exports.default = _default; require("./a"); function _default() { return 4; }

everything seems to be ok, a.js should be able to import/require it - as those anonymous default exports are splitted into an identifier + named function/class declaration

I suspect that there might be problems with non-hoisted by default things being splitted, right? Like in

// index.js import "./a"; export default 4; // a.js import four from "./index"; console.log(four)

this would actually end up as

// index.js exports.__esModule = true; exports.default = void 0; require("./a"); var _default = 4; exports.default = _default; // a.js var _index = _interopRequireDefault(require("./index")); function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; } console.log(_index.default);

and break - I'll work on fixing this later today. Thanks for catching this!

I can't tell from your comment if I was actually clear enough, so just to clarify:

The issue is that

import "./a"; function _default() { return 4; } export default _default;

exports the default as an expression, meaning the export default _default line must have been evaluated before the imported default function will be available, whereas

export { _default as default };

explicitly re-exports the _default binding, which means it is exported up front before the imports are processed.

Mostly It doesn't seem like there's anything to gain from this optimization.

export { _default as default };

is universally more consistent than

export default _default;

so essentially all we are gaining is a couple bytes.

It seems that we are still not on the same page, probably because I don't know spec that well as you do. I've tried to look into it right now to clarify things, but man, this thing is still too hard for me to comprehend.

I realize that I do not understand how import/export hoisting works exactly.

Let's look on the given example of:

import "./a"; function _default() { return 4; } export default _default;

gets transpiled by the master branch to

"use strict"; Object.defineProperty(exports, "__esModule", { value: true }); exports.default = void 0; require("./a"); function _default() { return 4; } var _default2 = _default; exports.default = _default2;

and on my PR to

"use strict"; Object.defineProperty(exports, "__esModule", { value: true }); exports.default = _default; require("./a"); function _default() { return 4; }

Which is actually incorrect, do I get it correct? Because we are exporting an expression it shouldnt be hoisted right away and assigned to exports.default.

It is correct by accident in tests etc because this is an output of splitting the:

export default function () { return 4; }

which should get hoisted and all, but after we do the split the intermediate output is wrong (or rather the intermediate output is wrongfully transformed into "correct" output).

I believe transforming into export { _default as default }; is correct approach IF _default should get hoisted. If we split export default 4 I'd argue that the current way (transforming into export default _default) is more correct, because it doesn't "allow" for changing value of _default binding afterwards and thus changing the exported value. Such export is not live.

Which is actually incorrect, do I get it correct? Because we are exporting an expression it shouldnt be hoisted right away and assigned to exports.default.

Yep, that's right.

I'd argue that the current way (transforming into export default _default) is more correct, because it doesn't "allow" for changing value of _default binding afterwards and thus changing the exported value. Such export is not live.

It seems like an overoptimization to me. If some other plugin is mutating bindings locally, they already run the risk of mutating a live binding of something they weren't supposed to if the user wrote their code without ever reassigning an exported variable. Also it seems counter to what I mentioned in my other comment for what the overall goal of this module should be. I view the goal of splitting specifically to separate an export into a standard JS statement with no export info, and an export statement without runtime behavior. By splitting it into export default foo;, we've split one statement with an expression into two separate statements with expressions.

babel-bot · 2018-02-02T10:41:12Z

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

loganfsmyth · 2018-02-02T20:40:02Z

packages/babel-helper-split-export-declaration/src/index.js

+    return t.exportSpecifier(t.identifier(name), t.identifier(name));
+  });
+
+  if (specifiers.length) {


If there aren't specificers, should we throw an exception? Seems like there should always be some. If this is intended to handle export { foo } from "bar"; we should probably bail out if there is a source instead.

I've just extracted this functionality for reuse in other places, so I'm not sure what this if was supposed to handle.

I've added a throw in there and tests did pass, so I'm going to remove this if - this helper is intended to actually split things and is not supposed to do maybeSplit*, so I actually prefer not to swallow any cases here.

As to export { foo } from "bar"; - this case is handled few lines above with:

if (exportDeclaration.get("specifiers").length > 0) { throw new Error("It doesn't make sense to split exported specifiers."); }

this case is handled few lines above with:

oh so it is, good call

loganfsmyth · 2018-02-02T20:43:19Z

packages/babel-helper-split-export-declaration/README.md

+## API
+
+```js
+declare export default splitExportDeclaration(path: NodePath);


this was intended as API doc - I've followed what was already done in example here

Alright, I wondered if it was done somewhere else. I guess it's fine, I don't feel strongly. Just potentially distracting since not everyone knows Flowtype.

loganfsmyth · 2018-02-02T20:55:27Z

packages/babel-helper-split-export-declaration/src/index.js

+
+  if (isDefault) {
+    if (declaration.isIdentifier()) {
+      throw new Error("It doesn't make sense to split exported identifier.");


Is this strictly necessary? It seems like we'd be able to change the logic for generating CmmonJS separately so

class Foo {} export { Foo };

still transforms to something readable, even when it is split.

What would be the value of splitting this? It would end up being something like

class Foo {} let Foo2 = Foo export { Foo2 as Foo };

I've explicitly forbidden those cases as I'm throwing for them here, both for ExportDefaultDeclaration and ExportNamedDeclaration.

If you feel like those should be allowed, I'm really curious about when it could be useful.

Yeah sorry, not sure what I was thinking when I wrote this. I do have a comment here, but I don't think this comment was actually it haha.

My question here is, what do we gain by throwing? There's nothing inherently wrong with splitting

export default foo;

into

var _default = foo; export default _default;

and if the user doesn't want that they can already special-cause it before calling split.

I don't think there is anything inherently wrong with this, I just don't see any gains in splitting this. I've added error to guard users against blindly using this helper and ending up with weirdo outputs - especially that helpers are mainly used as internal packages from what I understand.

I also don't actually feel about this really strongly, so if you feel that this error is unnecessary let me know and I'll remove it.

I see the goal of splitting as "give me a standalone declaration that isn't tied to export. I'm curious what you see the goal as, if not that. Given that goal, whether the exported thing is an identifier or function/class declaration isn't really a special case.

I've focused more on the reasons why the splitting was already used - to simplify rest of the transform. Will change it to allow identifiers.

loganfsmyth · 2018-02-02T21:03:17Z

packages/babel-helper-module-transforms/src/rewrite-live-references.js

@@ -29,13 +30,24 @@ export default function rewriteLiveReferences(
  }

  for (const [local, data] of metadata.local) {
+    const names = data.bindings.map(({ name }) => name);


Could we avoid all of this if we adjusted rewriteBindingInitVisitor instead? e.g.

export default foo;

to

var _tmp = foo; export { _tmp as default };

then in rewriteBindingInitVisitor we could say if it's a var that is never reassigned, instead of

var _tmp = foo; exports.default = _tmp;

do

exports.default = foo;

loganfsmyth · 2018-02-03T21:23:10Z

packages/babel-helper-split-export-declaration/src/index.js

@@ -0,0 +1,65 @@
+import * as t from "@babel/types";
+
+const splitExportDeclaration = exportDeclaration => {


Style-wise, I find splitting up the export default splitExportDeclaration; and this harder to follow. Any reason not to use export default function splitExportDeclaration(exportDeclaration) { instead?

Agreed, no idea why I didn't use a one-liner here. Gonna change it soon.

Andarist · 2018-02-12T09:48:14Z

@loganfsmyth @nicolo-ribaudo would love your review on the updated version.

We still can end up with some unnecessary temp variables when using modules transforms - ie:

var _default = [];
exports.default = _default;"

or

const foo = 'foo';
var _default = foo;
exports.default = _default;

but this PR hasn't changed anything regarding this. I've tried to use @loganfsmyth suggestion from here but because of scope bindings being WAY out of sync after multiple transformations, I've ditched it for now.

loganfsmyth · 2018-02-12T17:19:13Z

packages/babel-traverse/src/scope/index.js

@@ -210,7 +210,7 @@ export default class Scope {
   * Generate a unique identifier and add it to the current scope.
   */

-  generateDeclaredUidIdentifier(name: string = "temp") {


Should we mark these name?: string so it is clear they are still optional? At a glance, this kind of looks like you've made them required, which isn't the case.

Sure thing, kinda forgot this part of the codebase is typed. Gonna fix soon.

loganfsmyth · 2018-02-12T17:20:31Z

I've tried to use @loganfsmyth suggestion from here but

I'm 100% fine with that being some later optimization.

loganfsmyth

Looks good beyond 1 nitpick.

Andarist added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 2, 2018

Andarist commented Feb 2, 2018

View reviewed changes

loganfsmyth reviewed Feb 2, 2018

View reviewed changes

loganfsmyth reviewed Feb 3, 2018

View reviewed changes

Andarist force-pushed the split-export-declaration branch 3 times, most recently from f3a2917 to a281847 Compare February 12, 2018 09:35

loganfsmyth reviewed Feb 12, 2018

View reviewed changes

loganfsmyth approved these changes Feb 12, 2018

View reviewed changes

Added babel-helper-split-export-declaration

2369846

Andarist force-pushed the split-export-declaration branch from a281847 to 2369846 Compare February 12, 2018 20:32

existentialism approved these changes Feb 13, 2018

View reviewed changes

Andarist merged commit 4d164bd into babel:master Feb 13, 2018

Andarist deleted the split-export-declaration branch February 13, 2018 15:44

aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018

Added babel-helper-split-export-declaration (babel#7313)

dceb9c4

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added babel-helper-split-export-declaration #7313

Added babel-helper-split-export-declaration #7313

Andarist commented Feb 2, 2018

Andarist Feb 2, 2018

Andarist Feb 2, 2018

nicolo-ribaudo Feb 2, 2018

loganfsmyth Feb 2, 2018

Andarist Feb 3, 2018 •

edited

Loading

loganfsmyth Feb 3, 2018

Andarist Feb 4, 2018

loganfsmyth Feb 4, 2018

babel-bot commented Feb 2, 2018 •

edited

Loading

loganfsmyth Feb 2, 2018 •

edited

Loading

Andarist Feb 3, 2018

loganfsmyth Feb 3, 2018

loganfsmyth Feb 2, 2018

Andarist Feb 3, 2018 •

edited

Loading

loganfsmyth Feb 3, 2018

loganfsmyth Feb 2, 2018

Andarist Feb 3, 2018

loganfsmyth Feb 3, 2018

Andarist Feb 4, 2018

loganfsmyth Feb 4, 2018

Andarist Feb 4, 2018

loganfsmyth Feb 2, 2018 •

edited

Loading

loganfsmyth Feb 3, 2018

Andarist Feb 4, 2018

Andarist commented Feb 12, 2018

loganfsmyth Feb 12, 2018

Andarist Feb 12, 2018

loganfsmyth commented Feb 12, 2018

loganfsmyth left a comment

		@@ -0,0 +1,65 @@
		import * as t from "@babel/types";

		const splitExportDeclaration = exportDeclaration => {

Added babel-helper-split-export-declaration #7313

Added babel-helper-split-export-declaration #7313

Conversation

Andarist commented Feb 2, 2018

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

Andarist Feb 3, 2018 • 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

babel-bot commented Feb 2, 2018 • edited Loading

loganfsmyth Feb 2, 2018 • 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

Andarist Feb 3, 2018 • 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

loganfsmyth Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganfsmyth commented Feb 12, 2018

loganfsmyth left a comment

Choose a reason for hiding this comment

Andarist Feb 3, 2018 •

edited

Loading

babel-bot commented Feb 2, 2018 •

edited

Loading

loganfsmyth Feb 2, 2018 •

edited

Loading

Andarist Feb 3, 2018 •

edited

Loading

loganfsmyth Feb 2, 2018 •

edited

Loading