Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 106 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
106 commits
Select commit Hold shift + click to select a range
8296965
Add a spec mode to transform-es2015-modules-commonjs
Dec 8, 2016
4a746ee
Correct the descriptors to have writable: true
Dec 8, 2016
43bd0e9
Update tests
Dec 8, 2016
3389e9f
Simplify the specRequireInterop helper; spec isn't ES3 compatible
Dec 8, 2016
17cd0b9
Add missing @@toStringTag definition
Dec 8, 2016
42d870e
Fix broken AST attachment; update tests
Dec 8, 2016
477cb99
Use a check that won't actually throw in ES5 strict mode
Dec 8, 2016
6fbf2ac
Update fixtures
Dec 8, 2016
f15b506
Merge branch 'master' into modules-commonjs-spec
Dec 8, 2016
906e8c1
Don't set writable alongside getters
Dec 8, 2016
b88c6c5
Shorten generated code
Dec 8, 2016
8f18792
Update fixtures
Dec 8, 2016
60900bd
Fix several corner cases in the commons shadowing
Dec 8, 2016
a57449f
Add more fixtures, update fixtures
Dec 8, 2016
d09ac54
Better check for typeof Symbol
Dec 8, 2016
cf5e102
Avoid allocation on non-spec
Dec 8, 2016
538a585
Update fixtures
Dec 8, 2016
ba377a9
Try to be nicer to es5-sham
Dec 9, 2016
adea187
Remove unneeded unary negation
Dec 9, 2016
62ade30
Try to convert access to "global" exports / module into TDZ violations
Dec 9, 2016
418ee6b
Update fixtures
Dec 9, 2016
4afaba8
Add moduleSpec option to preset-es2015
Dec 9, 2016
ad9fc26
Correct logic error
Dec 9, 2016
b37066f
Use getter on default export of named class or function
Dec 9, 2016
cf7a7e4
Add an easy to read import test case with all import forms
Dec 9, 2016
0efc438
Add a test that actually executes the module body and inspects the ex…
Dec 9, 2016
ebfb274
Correct bugs found by the runtime test
Dec 9, 2016
c083987
Update fixtures
Dec 9, 2016
0c2bd66
Missing fixture update
Dec 10, 2016
6fd580d
Add (failing) reexport runtime tests
Dec 10, 2016
2e0f740
Hoist all exports in spec mode, avoid name clashes on star reexport
Dec 10, 2016
c428c52
Update fixtures
Dec 10, 2016
f3b8e11
Refactor runtime tests
Dec 10, 2016
f18e5dc
Remove unused template, update comments
Dec 10, 2016
56164e7
Remove the hoistedExports.has checks; babylon already checks for this
Dec 10, 2016
6340b24
Add more specific runtime tests for the export shape
Dec 10, 2016
f6c014a
Improve runtime tests
Dec 10, 2016
a861f64
Add reexport tests
Dec 10, 2016
9ec0147
Update fixtures
Dec 10, 2016
de10efb
Try to detect extra transforms needed for node 0.12
Dec 10, 2016
2338252
Actually fix tests to run in node 0.12
Dec 10, 2016
cd388c9
Fix incorrect arguments to array push
Dec 10, 2016
3f2f1a7
Refactor test/spec-interop-import
Dec 10, 2016
f9dd485
Add missing this.skip()
Dec 10, 2016
f27fe08
Also hoist the declaration of the default export
Dec 10, 2016
2fddb7a
Update fixtures
Dec 10, 2016
ba4d3d6
Also hoist the Object.freeze call
Dec 10, 2016
82455bc
Update fixtures
Dec 10, 2016
33a956d
Use before hooks to run less code during TEST_GREP
Dec 10, 2016
db72c87
Hoist the initialization of the default func if it's inside the funct…
Dec 10, 2016
8ba476d
Add default func hoisting fixture, update fixtures
Dec 10, 2016
97db049
Add documentation for spec mode
Dec 11, 2016
31205a8
Add a 'specImport' option to only enable the import half of spec mode
Dec 11, 2016
f6f291f
Improve README
Dec 11, 2016
1861e4d
Use const for specImport namespaces to ensure TDZ in circular references
Dec 11, 2016
a371bb1
Add more specImport tests, update fixtures
Dec 11, 2016
d8c8e86
Keep track of used import names; assert that they exist at import time
Dec 11, 2016
c920f26
Update tests and fixtures
Dec 11, 2016
03c6ef9
Add missing before handlers
Dec 11, 2016
4d54fdc
Don't give the module name as argument to helper; would have prevente…
Dec 11, 2016
76eea10
Update README
Dec 11, 2016
8aa75b5
Fix typo in helper
Dec 11, 2016
a8b094c
Add runtime tests for the import name checker
Dec 11, 2016
157ab45
Update fixtures
Dec 11, 2016
79a87ca
Copy the _blockHoist in non-spec specImport mode, just to make sure
Dec 11, 2016
0a6ead9
Helpers can't use const
Dec 11, 2016
b2cb529
Add a section about star reexports to the README
Dec 11, 2016
5eb912f
Add the README snippets as fixtures; use (reformatted) output in the …
Dec 11, 2016
52f9b5b
Give a better example for the options :blush:
Dec 11, 2016
60c0e9c
Add note about TDZ
Dec 11, 2016
79941d8
Use faster filter, use JSON.stringify for the list of invalid imports
Dec 11, 2016
a95a651
Update tests, correct invalid second argument
Dec 11, 2016
a77935c
Fix incorrect second argument to assert.throws in tests
Dec 11, 2016
6dbd228
Add specNamespaceGet helper, use when doing computed access to namespace
Dec 11, 2016
789aeae
Add fixture and runtime tests
Dec 11, 2016
5f62061
Add test to verify __esModule is considered an unknown export
Dec 11, 2016
3b7f443
Add test to document the namespace get wrapping can't be perfect
Dec 11, 2016
c6aec5f
Add non-spec test to verify export * from ordering behavior
Dec 12, 2016
d212a37
Remove _blockHoist hack for imports; just remove check later if it's …
Dec 12, 2016
77ae141
Update fixtures
Dec 12, 2016
fe39f70
Use names in readme snippet that generateUidIdentifier likes better
Dec 12, 2016
d6ce6ab
Update README
Dec 12, 2016
df80908
Remove unneeded argument
Dec 12, 2016
2241a88
Check if renamed reexports are correctly exported
Dec 12, 2016
9989e97
Add missing before hooks; fix tests failing on Node < 6
Dec 12, 2016
33f2325
Use propertyIsEnumerable instead of getOwnPropertyDescriptor
Dec 12, 2016
ca4b63b
Use better SameValue algorithm
Dec 12, 2016
65680c9
Update fixtures
Dec 12, 2016
70d0f9a
Use a helper for the SameValue check
Dec 12, 2016
e233d40
Update fixtures
Dec 12, 2016
27474f6
Merge branch 'master' into modules-commonjs-spec
Dec 12, 2016
1d63392
Fix crash when processing ThisExpression
Dec 12, 2016
ccd3895
Add failing test (fix TODO)
Dec 12, 2016
ba41759
Fix order in options.json; test still failing
Dec 12, 2016
838fb7b
Allow the helperGenerator hook to return the helper itself
Dec 12, 2016
6097372
transform-runtime: flag blacklisted helpers for skipping transform
Dec 12, 2016
ba6dda0
Build the babel-runtime dist scripts from current code
Dec 12, 2016
1c5bbcc
Huge refactoring; reimplement spec / specImport modes with visitors
Dec 12, 2016
d9336f7
Remove unused variable, update fixture
Dec 12, 2016
6066a22
Avoid calling scope.getBinding on Flow types
Dec 14, 2016
6fa02a8
Convert spec-test-helpers to ES6
Dec 14, 2016
d50b59d
Be more proactive with the Invalid import throw
Dec 14, 2016
4d96719
Remove accidental !
Dec 15, 2016
9c6550e
Merge branch 'master' into modules-commonjs-spec
Jan 19, 2017
7b222ac
Fix SyntaxError
Jan 19, 2017
195d824
Fix invalid this
Jan 19, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/babel-core/src/transformation/file/index.js
Expand Up @@ -288,14 +288,14 @@ export default class File extends Store {

const generator = this.get("helperGenerator");
const runtime = this.get("helpersNamespace");
const res = generator && generator(name);
if (generator) {
const res = generator(name);
if (res) return res;
if (res && t.isIdentifier(res)) return res;
Copy link
Member

Choose a reason for hiding this comment

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

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

} else if (runtime) {
return t.memberExpression(runtime, t.identifier(name));
}

const ref = getHelper(name);
const ref = res || getHelper(name);
const uid = this.declarations[name] = this.scope.generateUidIdentifier(name);

if (t.isFunctionExpression(ref) && !ref.id) {
Expand Down
90 changes: 90 additions & 0 deletions packages/babel-helpers/src/helpers.js
Expand Up @@ -396,6 +396,9 @@ helpers.interopRequireDefault = template(`
})
`);

// interopRequireDefault doesn't technically match spec,
// but where it does not match is not observable

helpers.interopRequireWildcard = template(`
(function (obj) {
if (obj && obj.__esModule) {
Expand All @@ -413,6 +416,93 @@ helpers.interopRequireWildcard = template(`
})
`);

helpers.specRequireInterop = template(`
(function (obj) {
if (obj && obj.__esModule) {
return obj;
} else {
var newObj = Object.create ? Object.create(null, {
default: {
value: obj,
writable: true,

Choose a reason for hiding this comment

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

Is this necessary? newObj is frozen soon, anyway.

enumerable: true
},
__esModule: {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@Jessidhia Jessidhia Dec 8, 2016

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

value: true
}
}) : {
default: obj,
__esModule: true
};
if (typeof Symbol === "function" && Symbol.toStringTag) {
Object.defineProperty(newObj, Symbol.toStringTag, { value: "Module" })
}
return (Object.freeze || Object)(newObj);
}
})
`);

helpers.specImportCheck = template(`
(function (module, imports) {
if (!module.__esModule) throw new Error("Only ES modules can be checked");
Copy link
Member

Choose a reason for hiding this comment

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

This means you can only import things that babel produced - that doesn't seem right at all. the "spec" option is useless if it forbids you from importing CJS modules.

Copy link
Member Author

@Jessidhia Jessidhia Dec 11, 2016

Choose a reason for hiding this comment

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

The specInteropRequire that is used in all imports wraps CJS modules in an ES module. The result that only 'default' will be accepted.

Copy link
Member

Choose a reason for hiding this comment

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

aha, ok, makes sense

var invalid = imports.filter(function (i) { return !Object.prototype.propertyIsEnumerable.call(module, i) });
if (invalid.length > 0) {
var error = new Error(
"Unknown export" + (invalid.length > 1 ? "s " : " ") +
JSON.stringify(invalid) +
" imported"
);
error.module = module;
throw error;
}
})
`);

// The name && typeof Symbol === "function" && ... line is from helpers.typeof, which may be needed
// when polyfilled. Helpers might not be transformed when using external-helpers, so can't rely on
// the typeof being augmented when targeting ES5.
helpers.specNamespaceGet = template(`
(function (module, name) {
if (!module.__esModule) throw new Error("Only ES modules can be checked");
if (
typeof name === "symbol" ||
name && typeof Symbol === "function" && name.constructor === Symbol && name !== Symbol.prototype
) {
return module[name];
}
if (!Object.prototype.propertyIsEnumerable.call(module, name)) {
throw new Error("Unknown export " + JSON.stringify(name) + " imported");
}
return module[name];
})
`);

// sameValue function based on core-js's SameValue implementation
// https://github.com/zloirock/core-js/blob/693767b/modules/_same-value.js
helpers.specNamespaceSpread = template(`
(function (exports, ownExports, module) {
if (!module.__esModule) throw new Error("Only ES modules can be spread");
for (var key in module) {
if (!Object.prototype.hasOwnProperty.call(module, key)) continue;
if (key === "__esModule" || key === "default" || ownExports.indexOf(key) >= 0) continue;
if (key in exports && sameValue(exports[key], module[key])) continue;

Object.defineProperty(exports, key, {
enumerable: true,
get: (function (key) {
return function () {
return module[key];
}
})(key)
});
}

function sameValue(x, y) {
return x === y ? x !== 0 || 1 / x === 1 / y : x != x && y != y;
}
})
`);

helpers.newArrowCheck = template(`
(function (innerThis, boundThis) {
if (innerThis !== boundThis) {
Expand Down
134 changes: 133 additions & 1 deletion packages/babel-plugin-transform-es2015-modules-commonjs/README.md
Expand Up @@ -42,7 +42,7 @@ npm install --save-dev babel-plugin-transform-es2015-modules-commonjs
{
"plugins": [
["transform-es2015-modules-commonjs", {
"allowTopLevelThis": true
"spec": true
}]
]
}
Expand All @@ -62,6 +62,125 @@ require("babel-core").transform("code", {
});
```

## Options `spec`

By default, `babel` actually implements importing very loosely, by
supporting treating a commonjs export as if it was a namespace export.
The exported namespace is also not frozen and has an incorrect prototype.

The `spec` option, when set to `true`, tries to generate code that is as
close as possible to what is required by the ECMA262 spec without relying
on `Proxy`. The exports will be frozen, and imports will always be treated
like ES modules. All imported names will be checked, at import time, if
they exist in the exports of the imported module. If `let` and `const` are
not transformed to `var`, the exports will also implement TDZ.

Importing a commonjs module (say, the standard `fs` module) will always
wrap it in an ES module that has a single `default` export. This means that
some imports that work in non-`spec` mode, like `import { readFile } from 'fs'`,
Copy link
Member

Choose a reason for hiding this comment

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

This won't necessarily be true if we end up with interop for named CJS exports - @bmeck, is this still a possibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible. Spec does not explicitly ban it from happening. Implementations have concerns and order of operations may be hard for codegen. Still in talks.

will result errors in `spec` mode.

Note that exports, under this mode, always require runtime support for
getters. It 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.

### Input

```javascript
import 'a';
import defaultImport from 'b';
import * as namespace from 'c';
import { pick } from 'd';

defaultImport(namespace.foo, pick);

export { pick }
export default function () {}
```

### Output

```javascript
'use strict';

const exports = module.exports = Object.create ? Object.create(null, {
__esModule: {
value: true
}
}) : {
__esModule: true
};
if (typeof Symbol === "function" && Symbol.toStringTag) {
Object.defineProperty(exports, Symbol.toStringTag, {
value: "Module"
});
}

Object.defineProperties(exports, {
pick: { enumerable: true, get() { return _d.pick; } },
default: { enumerable: true, get() { return _default; } }
});
let _default = {
default: function () {}
}.default;
(Object.freeze || Object)(exports);
Copy link
Member

Choose a reason for hiding this comment

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

if you're requiring getters, then Object.freeze will always be available.


require('a');

const _b = babelHelpers.specRequireInterop(require('b'));

babelHelpers.specImportCheck(_b, ['default']);

const _c = babelHelpers.specRequireInterop(require('c'));

babelHelpers.specImportCheck(_c, ['foo']);

const _d = babelHelpers.specRequireInterop(require('d'));

babelHelpers.specImportCheck(_d, ['pick']);


(0, _b.default)(_c.foo, _d.pick);
```

## Options `specImport`

This option enables only the half of `spec` mode that affects the imports, without
changing how exports are generated. This would allow the generation of code that
may still be compatible with engines that do not support getters.

Note that the require helpers do use `Object.getOwnPropertyDescriptor` and
`Object.defineProperty`, so ES5 polyfills may still be required. When running on an
old engine that does not support `Object.defineProperty`, a polyfill to fake it like
`es5-sham` is required.

This option is **ignored** if `spec` is enabled. Enabling `spec` implies that this
option is also enabled.

### Input

```javascript
import { pick } from 'module'

export default pick()
```

### Output

```javascript
'use strict';

Object.defineProperty(exports, "__esModule", {
value: true
});

const _module = babelHelpers.specRequireInterop(require('module'));

babelHelpers.specImportCheck(_module, ['pick']);
exports.default = (0, _module.pick)();
```

## Options `loose`

As per the spec, `import` and `export` are only allowed to be used at the top
Expand All @@ -85,3 +204,16 @@ and instead of using `Object.defineProperty` an assignment will be used instead.
var foo = exports.foo = 5;
exports.__esModule = true;
```

The `loose` option is **ignored** if used in combination with `spec`.

## Caveats

Star reexports (`export * from 'module'`) always run before other imports or
reexports, as this module's exports must be known before other modules execute.
That is, they behave as if there was an `import 'module'` that runs before any
of the other imports.

This is particularly hard to avoid in the `spec` / `specImport` mode, as the
exports must be frozen before importing other modules. Star reexports are the
only exception.