Add a spec mode to transform-es2015-modules-commonjs #4964

Open
wants to merge 106 commits into
from

Conversation

Projects
None yet
8 participants
@Kovensky
Member

Kovensky commented Dec 8, 2016

Q A
Bug fix? no
Breaking change? no
New feature? yes
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets
License MIT
Doc PR
Dependency Changes

This adds a spec: true mode to transform-es2015-modules-commonjs, which tries to make the exports behave as close as possible to the specification for ES2015 modules.

export default will not create an identifier at all. Other exports are done by creating live bindings using a getter, instead of injecting assignments to exports in the middle of every mutation.

export default of an anonymous function or class will wrap the export in an { default: $0 }.default inline object + member access to ensure the Function#name is correctly set (as close as possible) to default, instead of becoming value.

All imports (except for side-effect-only imports) must be processed through the new specRequireInterop helper, that will return a (hopefully) fully compliant module record wrapper for commonjs modules. This will make enabling spec mode a breaking change for users that "destructure" commonjs object exports, or import them in any other way than a default import.

This will also prevent access to the exports or module.exports objects from inside user code, to avoid unknown code tinkering with the state of the exports. Attempts to access them will access uids instead, and by the time other modules can access the export object, it will have already been through Object.freeze.

This is incompatible with loose or strict mode (whatever strict mode is; it just avoids adding the __esModule export?). The point of loose is avoiding ES5-only APIs, but it's not possible to implement spec without using Object.create or Object.defineProperty. Luckily, we don't need Object.setPrototypeOf 馃槄

+});
+const _undefined = {
+ enumerable: true,
+ configurable: true,

This comment has been minimized.

@Kovensky

Kovensky Dec 8, 2016

Member

Oops, I guess these (and the regular records as well) should be writable: true instead.

While the export record itself is not really defined, when observed from an importing module, it is defined to be writable: true.

@Kovensky

Kovensky Dec 8, 2016

Member

Oops, I guess these (and the regular records as well) should be writable: true instead.

While the export record itself is not really defined, when observed from an importing module, it is defined to be writable: true.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 8, 2016

Current coverage is 89.37% (diff: 96.90%)

No coverage report found for master at 3871236.

Powered by Codecov. Last update 3871236...4d96719

codecov-io commented Dec 8, 2016

Current coverage is 89.37% (diff: 96.90%)

No coverage report found for master at 3871236.

Powered by Codecov. Last update 3871236...4d96719

Kovensky added some commits Dec 8, 2016

Merge branch 'master' into modules-commonjs-spec
* master:
  update `regenerator-runtime` in `babel-polypill` (#4966)
  Temp fix for make watch [skip ci] (#4967)
  Add (and fix) failing test of function parameter bindings in a catch block (#4880)
  Upgrade regenerator-runtime to version 0.10.0. (#4877)
  Add `/.test` and `/src` to `babel-plugin-transform-regenerator` `.npmignore`. (#4961) [skip ci]
  Only base async fn arity on non-default/non-rest params - fixes #4891 (#4901)
  Add generator support for Import (#4945)
Don't set writable alongside getters
Throws in v8 with "Invalid property descriptor. Cannot both specify
accessors and a value or writable attribute"

I guess this is why the spec defines "magic changing values" instead of
getters 馃槄
Shorten generated code
Repeat the shorter identifier in the block.
packages/babel-helpers/src/helpers.js
+ value: true
+ }
+ });
+ if (typeof Symbol !== "undefined" && Symbol.toStringTag) {

This comment has been minimized.

@ljharb

ljharb Dec 8, 2016

To be even stricter, this should check that Symbol is a function.

@ljharb

ljharb Dec 8, 2016

To be even stricter, this should check that Symbol is a function.

+ writable: true,
+ enumerable: true
+ },
+ __esModule: {

This comment has been minimized.

@ljharb

ljharb Dec 8, 2016

Is it worth also tagging this somehow as a spec __esModule, just in case a future iteration of the helper wants to care about it?

@ljharb

ljharb Dec 8, 2016

Is it worth also tagging this somehow as a spec __esModule, just in case a future iteration of the helper wants to care about it?

This comment has been minimized.

@Kovensky

Kovensky Dec 8, 2016

Member

If we assume that nothing actually checks for __esModule equality (only definedness / truthy-ness), it could be set to value: 'spec'; but it's probably not a good idea to add even more nonstandard keys...

@Kovensky

Kovensky Dec 8, 2016

Member

If we assume that nothing actually checks for __esModule equality (only definedness / truthy-ness), it could be set to value: 'spec'; but it's probably not a good idea to add even more nonstandard keys...

This comment has been minimized.

@ljharb

ljharb Dec 8, 2016

yeah, that's true. it was just a thought - it can probably be inferred with Object.isFrozen or checking the descriptor.

@ljharb

ljharb Dec 8, 2016

yeah, that's true. it was just a thought - it can probably be inferred with Object.isFrozen or checking the descriptor.

This comment has been minimized.

@bmeck

bmeck Dec 9, 2016

Contributor

I would keep __esModule, you can check enumerability here to double check it is a "fake" es module. I am also concerned to keep w/ things that were already compiled in babel from the past.

@bmeck

bmeck Dec 9, 2016

Contributor

I would keep __esModule, you can check enumerability here to double check it is a "fake" es module. I am also concerned to keep w/ things that were already compiled in babel from the past.

This comment has been minimized.

@ljharb

ljharb Dec 9, 2016

I was originally suggesting having two keys - keeping __esModule, and adding __esSpecModule or something. Checking enumerability to infer __esSpecModule is fine, although not as nicely explicit imo as a second special property.

@ljharb

ljharb Dec 9, 2016

I was originally suggesting having two keys - keeping __esModule, and adding __esSpecModule or something. Checking enumerability to infer __esSpecModule is fine, although not as nicely explicit imo as a second special property.

This comment has been minimized.

@Kovensky

Kovensky Dec 10, 2016

Member

The __esModule key is already not writable/enumerable/configurable in the regular transform; it's only enumerable with loose: true. I guess Object.isFrozen would be the only reliable way without needing Symbol.toStringTag

@Kovensky

Kovensky Dec 10, 2016

Member

The __esModule key is already not writable/enumerable/configurable in the regular transform; it's only enumerable with loose: true. I guess Object.isFrozen would be the only reliable way without needing Symbol.toStringTag

packages/babel-helpers/src/helpers.js
+ if (obj && obj.__esModule) {
+ return obj;
+ } else {
+ var newObj = Object.create(null, {

This comment has been minimized.

@ljharb

ljharb Dec 8, 2016

I think there should still be a fallback here for envs without Object.create - perhaps it could fall back to the non-spec interop helper?

@ljharb

ljharb Dec 8, 2016

I think there should still be a fallback here for envs without Object.create - perhaps it could fall back to the non-spec interop helper?

This comment has been minimized.

@Kovensky

Kovensky Dec 8, 2016

Member

This helper is only called when using the spec: true option, which can only generate exports using Object.defineProperty. I couldn't find any platform with a working Object.defineProperty but not a working Object.create with a quick look at compat-table...

@Kovensky

Kovensky Dec 8, 2016

Member

This helper is only called when using the spec: true option, which can only generate exports using Object.defineProperty. I couldn't find any platform with a working Object.defineProperty but not a working Object.create with a quick look at compat-table...

This comment has been minimized.

@Kovensky

Kovensky Dec 8, 2016

Member

Hm, it could potentially work with a spec + loose version that disables the commonjs shadowing and uses similar export logic to the non-spec version...

@Kovensky

Kovensky Dec 8, 2016

Member

Hm, it could potentially work with a spec + loose version that disables the commonjs shadowing and uses similar export logic to the non-spec version...

This comment has been minimized.

@ljharb

ljharb Dec 8, 2016

Anything ES3 that has es5-sham will work fine with all your uses of defineProperty, but will not work with Object.create(null).

My thinking is that the "spec" transform should of course make everything compliant when possible, but that in envs where it's not possible, it should still work.

@ljharb

ljharb Dec 8, 2016

Anything ES3 that has es5-sham will work fine with all your uses of defineProperty, but will not work with Object.create(null).

My thinking is that the "spec" transform should of course make everything compliant when possible, but that in envs where it's not possible, it should still work.

@@ -246,10 +385,15 @@ export default function () {
let defNode = t.identifier("default");
if (id) {
addTo(exports, id.name, defNode);
- topNodes.push(buildExportsAssignment(defNode, id));
+ topNodes.push(spec ? specBuildExportDefault({ EXPORTS: exportsObj, VALUE: id }) : buildExportsAssignment(defNode, id));

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 9, 2016

Member

These have the potential to still be live-binding if something assigns to id, will we need to use specBuildExport for that case?

@loganfsmyth

loganfsmyth Dec 9, 2016

Member

These have the potential to still be live-binding if something assigns to id, will we need to use specBuildExport for that case?

This comment has been minimized.

@Kovensky

Kovensky Dec 9, 2016

Member

Oh, right, brings me terrible memories of this function I saw in real code...

function template(selector, params) {
  template = $('#template-' + selector).html();
  return templateLib.replace(template, params);
}

I thought it was an error, but code actually relies on the function becoming a string after the first invocation.

@Kovensky

Kovensky Dec 9, 2016

Member

Oh, right, brings me terrible memories of this function I saw in real code...

function template(selector, params) {
  template = $('#template-' + selector).html();
  return templateLib.replace(template, params);
}

I thought it was an error, but code actually relies on the function becoming a string after the first invocation.

This comment has been minimized.

@loganfsmyth

loganfsmyth Dec 9, 2016

Member

oh god my eyes!

@loganfsmyth

loganfsmyth Dec 9, 2016

Member

oh god my eyes!

This comment has been minimized.

@Kovensky

Kovensky Dec 10, 2016

Member

It is now immortalized as a test case 馃槅

@Kovensky

Kovensky Dec 10, 2016

Member

It is now immortalized as a test case 馃槅

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Dec 9, 2016

Contributor

talked w/ @Kovensky. +1 as it looks like specRequireInterop is as close as possible to spec w/o resorting to Proxies and shipping a custom loader instead of using require under the hood.

Contributor

bmeck commented Dec 9, 2016

talked w/ @Kovensky. +1 as it looks like specRequireInterop is as close as possible to spec w/o resorting to Proxies and shipping a custom loader instead of using require under the hood.

Kovensky added some commits Dec 12, 2016

Merge branch 'master' into modules-commonjs-spec
* master:
  make installing runtime/transform-runtime clearer [skip ci] (#4991)
  Add example to es2015-unicode-regex [skip ci] (#4983)
  v6.20.3
  Calculate the correct arity for async functions with destructuring - fixes #4977 (#4978)
  v6.20.2
  fix object spread (#4976)
  fix clean lib
  update readme [skip ci]
  v6.20.1
  Fix nested object spread (#4974)
  v6.20.0
  v6.20.0 changelog [skip ci] (#4971)
  Raise limit on code size before compacting (#4965)
  Use regenerator-transform to implement babel-plugin-transform-regenerator (#4881)
  Add getBindingIdentifierPaths/getOuterBindingIdentifierPaths (#4876)
  Hoist generateDeclaredUidIdentifier helper function (#4934)
@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Dec 12, 2016

Contributor

if we are changing more than just the wrapper of CJS modules it might make sense to ensure the top level this is set to undefined : https://tc39.github.io/ecma262/#sec-module-environment-records-getthisbinding

though doing so would mean that all source texts (Script, CJS, ESM) would all display that behavior which most likely would break some things in CJS and Script that expect different this bindings. BabelJS to my knowledge does not attempt to discern what type of source text a given file is.

Contributor

bmeck commented Dec 12, 2016

if we are changing more than just the wrapper of CJS modules it might make sense to ensure the top level this is set to undefined : https://tc39.github.io/ecma262/#sec-module-environment-records-getthisbinding

though doing so would mean that all source texts (Script, CJS, ESM) would all display that behavior which most likely would break some things in CJS and Script that expect different this bindings. BabelJS to my knowledge does not attempt to discern what type of source text a given file is.

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Dec 12, 2016

Member

@bmeck looks like that's already implemented in the regular mode too by rewriting top level this to undefined -- but only if it was a this that was present in the original source.

It also turns out there is an undocumented option to skip the this rewriting...

Member

Kovensky commented Dec 12, 2016

@bmeck looks like that's already implemented in the regular mode too by rewriting top level this to undefined -- but only if it was a this that was present in the original source.

It also turns out there is an undocumented option to skip the this rewriting...

Kovensky added some commits Dec 14, 2016

Be more proactive with the Invalid import throw
Even if the path.remove did work, the import would have been deleted
without generating the corresponding require.
@loganfsmyth

Whew, I didn't realize just how big this PR had gotten. It's a little overwhelming.

- let res = generator(name);
- if (res) return res;
+ res = generator(name);
+ if (res && t.isIdentifier(res)) return res;

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

I don't totally follow the logic here, could you clarify for me, what's the result of changing this check?

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

I don't totally follow the logic here, could you clarify for me, what's the result of changing this check?

@@ -1,4 +1,5 @@
import definitions from "./definitions";
+import getHelper from "babel-helpers";

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

Hmm, somewhat hesitant to have this as a dependency since it means we now have two parallel sources of helpers.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

Hmm, somewhat hesitant to have this as a dependency since it means we now have two parallel sources of helpers.

@@ -19,6 +27,10 @@ export default function ({ types: t }) {
file.set("helperGenerator", function (name) {
if (HELPER_BLACKLIST.indexOf(name) < 0) {
return file.addImport(`${moduleName}/helpers/${name}`, "default", name);
+ } else {
+ const node = getHelper(name);

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

This is an attempt to avoid the import statements in the final output, like export * from, correct?

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

This is an attempt to avoid the import statements in the final output, like export * from, correct?

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Jan 19, 2017

Shouldn鈥檛 a default export also create an identifier? To support scenarios such as the following one:

export default function foo() {}
foo = 123;

IINM, for complete spec compliance, the local name (= value of property name) of a default-exported anonymous function declaration (or anonymous class declaration) should be '*default*', but the export name 'default'.

rauschma commented Jan 19, 2017

Shouldn鈥檛 a default export also create an identifier? To support scenarios such as the following one:

export default function foo() {}
foo = 123;

IINM, for complete spec compliance, the local name (= value of property name) of a default-exported anonymous function declaration (or anonymous class declaration) should be '*default*', but the export name 'default'.

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Jan 19, 2017

Member

@rauschma The export default specification for anonymous functions / classes (https://tc39.github.io/ecma262/#sec-exports-runtime-semantics-evaluation) does seem to use *default* as "spec magic" for the hidden "default export value", but in both cases there is a call to SetFunctionName(value, "default"). I can't directly name either the class or function "default", so I am using a wrapper like { default: function () {} }.default to get a contextual name.

As the exports are being frozen (Object.freeze), all exports are using getters. Even the default export (if an anonymous function / class / expression result) uses a unique var to hold its result, and is exported as a getter.

Member

Kovensky commented Jan 19, 2017

@rauschma The export default specification for anonymous functions / classes (https://tc39.github.io/ecma262/#sec-exports-runtime-semantics-evaluation) does seem to use *default* as "spec magic" for the hidden "default export value", but in both cases there is a call to SetFunctionName(value, "default"). I can't directly name either the class or function "default", so I am using a wrapper like { default: function () {} }.default to get a contextual name.

As the exports are being frozen (Object.freeze), all exports are using getters. Even the default export (if an anonymous function / class / expression result) uses a unique var to hold its result, and is exported as a getter.

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Jan 19, 2017

Thanks for the answer!

  • 鈥渂ut in both cases there is a call to SetFunctionName(value, "default")鈥.
    Yes, true, sorry!
  • 鈥淚 can't directly name either the class or function鈥
    The name property is not writable, but configurable. Should be possible(?)
  • 鈥渁ll exports are using getters鈥
    Right. I went by the first comment on this page, but have since found the readme, which helped much with understanding your approach.

Thanks for the answer!

  • 鈥渂ut in both cases there is a call to SetFunctionName(value, "default")鈥.
    Yes, true, sorry!
  • 鈥淚 can't directly name either the class or function鈥
    The name property is not writable, but configurable. Should be possible(?)
  • 鈥渁ll exports are using getters鈥
    Right. I went by the first comment on this page, but have since found the readme, which helped much with understanding your approach.
@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Jan 19, 2017

Member

Oh, I never actually tried using defineProperty to set a function's name; didn't know they were configurable (https://tc39.github.io/ecma262/#sec-function-instances-name) 馃槃

Going that way should actually help with the case of transform-es2015-function-name actually naming the functions _default. Possible idea for a spec mode for that as well.

Member

Kovensky commented Jan 19, 2017

Oh, I never actually tried using defineProperty to set a function's name; didn't know they were configurable (https://tc39.github.io/ecma262/#sec-function-instances-name) 馃槃

Going that way should actually help with the case of transform-es2015-function-name actually naming the functions _default. Possible idea for a spec mode for that as well.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 19, 2017

@Kovensky please do conditionally check for Object.defineProperty before calling it, for ES3 :-)

ljharb commented Jan 19, 2017

@Kovensky please do conditionally check for Object.defineProperty before calling it, for ES3 :-)

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Jan 19, 2017

Member

@ljharb this transform (in spec mode) already doesn't work if native Object.defineProperties isn't available (for getters)... though it is something to keep in mind if that change was done in transform-es2015-function-name

Member

Kovensky commented Jan 19, 2017

@ljharb this transform (in spec mode) already doesn't work if native Object.defineProperties isn't available (for getters)... though it is something to keep in mind if that change was done in transform-es2015-function-name

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Jan 19, 2017

Collaborator

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Collaborator

babel-bot commented Jan 19, 2017

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Jan 19, 2017

Collaborator

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Collaborator

babel-bot commented Jan 19, 2017

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 19, 2017

@Kovensky it can't gracefully degrade to not using getters in ES3 browsers? Most people don't use live bindings anyways.

ljharb commented Jan 19, 2017

@Kovensky it can't gracefully degrade to not using getters in ES3 browsers? Most people don't use live bindings anyways.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Jan 19, 2017

Collaborator

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Collaborator

babel-bot commented Jan 19, 2017

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Jan 19, 2017

Quoting the readme: 鈥淚t also is not possible to access or write to the commonjs exports or module objects; attempts to access them will result in TDZ errors at runtime.鈥

AFAICT by looking at the generated code, there will be TypeErrors, not a TDZ errors: const comes first and ends the TDZ.

Quoting the readme: 鈥淚t also is not possible to access or write to the commonjs exports or module objects; attempts to access them will result in TDZ errors at runtime.鈥

AFAICT by looking at the generated code, there will be TypeErrors, not a TDZ errors: const comes first and ends the TDZ.

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Jan 19, 2017

Wondering: shouldn鈥檛 module.exports be created via Object.defineProperty() and be non-writable, so that people can鈥檛 accidentally change its value?

Wondering: shouldn鈥檛 module.exports be created via Object.defineProperty() and be non-writable, so that people can鈥檛 accidentally change its value?

+ var newObj = Object.create ? Object.create(null, {
+ default: {
+ value: obj,
+ writable: true,

This comment has been minimized.

@rauschma

rauschma Jan 19, 2017

Is this necessary? newObj is frozen soon, anyway.

@rauschma

rauschma Jan 19, 2017

Is this necessary? newObj is frozen soon, anyway.

@hawkrives hawkrives referenced this pull request in babel/website Feb 7, 2017

Closed

Add FAQ for "import" vs destructuring #1156

@bmeck bmeck referenced this pull request in RReverser/esmod Mar 15, 2017

Closed

Replace TODO:{import,export} with proper implementations #2

@bmeck bmeck referenced this pull request in nodejs/node-eps Oct 13, 2017

Closed

.mjs extension trade-offs revision #57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment