ES6 Module mutable exports are not supported

[jplaisted]

// ./input0.js
export let X = 2;
export function foo() { X = 3 }

// ./input1.js
import { X, foo } from './input0.js';
console.log(X);
foo();
console.log(X);

Expected result: 2 3. Actual result: 2 2.

Should be obvious just looking at this. All local variables are rewritten to globals, and exports are assigned to said globals at the bottom of the module. What needs to happen is if an export is mutable all references to it need to reference the export; no local.

[jplaisted]

Slightly related: export from is also broken. Export from requires the above to be fixed and for the export from to use Object.defineProperty with a getter for the reexported property.

// ./input0.js
export let X = 2;
export function foo() { X = 3 }

// ./input1.js
export { X, foo } from './input0.js';

// ./input2.js
import { X, foo } from './input1.js';
console.log(X);
foo();
console.log(X);

[jplaisted]

Might be moot if we ban this. See #2717.

[jplaisted]

Another case we'll need to look out for is export from via import:

import { imported } from './file.js';
export { imported };

[jplaisted]

@ChadKillingsworth I'd like your input on this.

I'm thinking the only real way to support this in an efficient manner is to only have globals, which tl;dr breaks import * as ns from 'mod'; for (let key in ns) {};.

So the issue is that we need to use Object.defineProperties in order create module exports objects that support mutable exports.

Explanation: Exports can keep being reexported and the reexports reflect the original export. Example:

var original = 0;
export { original as O };
export function update() { original++; }

import { O as Alias } from 'first';
export { Alias as Alias1 };
export { O as Alias2 } from 'first';
export * from 'first';

import { O as original, update} from 'first';
import { Alias1, Alias2 } from 'second';
import * as second from 'second';
// 0 0 0 0
console.log(`${original} ${second.O} ${Alias1} ${Alias2}`);
update();
// 1 1 1 1
console.log(`${original} ${second.O} ${Alias1} ${Alias2}`);

The only correct way to create an exports object for the second module is like this:

var module$second$exports = {};
Object.defineProperties(module$second$exports, {
  O: { get: function() { return original$module$first; },
  update: { get: function() { return update$module$first; },
  Alias1: { get: function() { return original$module$first; },
  Alias2: { get: function() { return original$module$first; },
});

Which is icky and should be avoided. So the idea is if you never use that object (e.g. no import * or the only usage of the variable from import * is property accesses) then we can rewrite to globals instead.

var original$module$first = 0;
function update$module$first() { original$module$first++; }

// 0 0 0 0
console.log(`${original$module$first} ${original$module$first} ${original$module$first} ${original$module$first}`);
update$module$first();
// 1 1 1 1
console.log(`${original$module$first} ${original$module$first} ${original$module$first} ${original$module$first}`);

How does this impact most of the CommonJS code you write, if at all? Obviously we'd have to do this rewriting with CommonJS; I'm just not sure if you use the import * as ns; use(ns); case often. We know how to support it (Object.defineProperties) but for now I'd like to skip it. Specifically if I see a variable from an import * not used as a getprop I'd like to throw a not yet supported error.

[jplaisted]

And fwiw there'd be an easy work around: rather than import * as ns; iterate(ns) you'd instead export an object and do import { ns }; iterate(ns);

[ChadKillingsworth]

Given the CommonJS module:

module.exports = 'foobar';

If an ES6 module imported CommonJS, import * as ns from '/commonjs.js' would make ns be the module namespace, so most access would be ns.default === 'foobar'.

Inside CommonJS, if you import a ES6 module that exported *, it would like like: const ns = require('/es6.js'); console.log(ns.0, ns. Alias1);.

The rewriting you describe is how CommonJS already operates. Here's an example: https://github.com/google/closure-compiler/blob/master/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java#L342-L348

[jplaisted]

Not exactly what I'm talking about. Here's a better example:

closure-compiler/test/com/google/javascript/jscomp/CommandLineRunnerTest.java

Lines 1780 to 1807 in 3f954c1

	public void testProcessCJSWithES6Export() {
	args.add("--process_common_js_modules");
	args.add("--entry_point=app");
	args.add("--dependency_mode=STRICT");
	args.add("--language_in=ECMASCRIPT6");
	args.add("--module_resolution=NODE");
	setFilename(0, "foo.js");
	setFilename(1, "app.js");
	test(
	new String[] {
	LINE_JOINER.join("export default class Foo {", " bar() { console.log('bar'); }", "}"),
	LINE_JOINER.join(
	"var FooBar = require('./foo').default;",
	"var baz = new FooBar();",
	"console.log(baz.bar());")
	},
	new String[] {
	LINE_JOINER.join(
	"var module$foo={},",
	"Foo$$module$foo=function(){};",
	"Foo$$module$foo.prototype.bar=function(){console.log(\"bar\")};",
	"module$foo.default=Foo$$module$foo;"),
	LINE_JOINER.join(
	"var FooBar = module$foo.default,",
	" baz = new module$foo.default();",
	"console.log(baz.bar());")
	});
	}

export default class Foo{}

var FooBar = require('./foo').default;
var baz = new FooBar();

Currently:

var module$foo = {};
var Foo$$module$foo=function(){};
module$foo.default=Foo$$module$foo;
var FooBar = module$foo.default;
var baz = new Foo();

Proposed solution is that module$foo does not exist, but all of its properties do as globals. So we reference those variables when you access a property:

var default$$module$foo=function(){};
var FooBar = module$foo.default;
var baz = new FooBar();

Note that I'm not proposing changing CommonJS exports. Only references to ES6 module exports, which can exist in a CommonJS module.

So what this means is that this works:

var FooBar = require('/myes6module');
FooBar.accessProperty();

This does not, at least for now:

var FooBar = require('/myes6module');
// "Cannot implement yet" for now since FooBar is not LHS of getprop
use(FooBar);
// Ditto, but this is an actual example as to why. FooBar does not exist.
// Only the properties of FooBar exist as their own globals.
for (let k of FooBar) {}

So this means that import and require have different behavior for ES6 modules and CommonJS modules. CommonJS modules will still have their exports / module objects, ES6 won't.

import * as cjs from 'mycjs'; // or const cjs = require('mycjs');
import * as es6 from 'es6'; // or const es6 = require('es6');
use(cjs);
use(cjs.foo);
// use(es6) is an error for now
use(es6.bar);

use(module$cjs);
use(module$cjs.foo);
use(bar$module$es6);

I've written a document that hopefully clarifies things as well.

[jplaisted]

So if we want to change how ES6 modules are rewritten then that means that I will need to move rewriting of references to ES6 modules via require() into Es6RewriteModules, and move rewriting references to CommonJS modules via import to ProcessCommonJSModules.

[ChadKillingsworth]

Right - sorry I missed some of the nuance until later.

This affects rewriting type nodes as well. I had originally thought we should have a shared class that can handle the rewriting of an import.

FYI this affects rewriting JSDoc types as well:

const Foo = require('/path/to/es6.js');
/**
 * @param {!Foo} foo
 * @param {!/path/to/es6} foo2
 */
function (foo, foo2) {}

[jplaisted]

Right, but why would you ever write a function like that exactly? Is it a common case?

[ChadKillingsworth]

Two reasons:

1. you have a linter that complains about unused variables and you need to reference a type without actually importing it.
2. You need to support circular references. This case may be fixed now with the proper ES6 module ordering, but I haven't verified that.

[ChadKillingsworth]

I don't think it's super common, but I know there are references like that in my code base. I believe there are references like that inside Google as well.

[ChadKillingsworth]

One more thing: you'll need to have a strategy for per-file transpilation. Right now for CommonJS, if the import module can't be located, I assume that the imported module is the same type (CommonJS). Just need to define this behavior.

Per-file transpilation is becoming harder and harder to support.

[jplaisted]

you have a linter that complains about unused variables and you need to reference a type without actually importing it.

I'm still not clear how your example function gets around this. You're importing in your example. imo @param {Foo} and @param {!/path/to/es6} don't really make sense. Again why are you using the whole module object? Use @param {Foo.RealType} or @param {!/path/to/es6:RealType} (not the current syntax but I'm going to fix that) instead. I don't like treating modules as types that can be passed around tbh.

I thought you didn't use per file transpilation?

To be clear what per file transpilations means: literally transpiling that one file with no other inputs. This is what ClosureBundler does. It is not what whitespace only or hotswap does. At least I'm guessing with hotswap that when it goes to hotswap the inputs still exist from the first compile, so you should be able to find the other modules. At least I hope. It not each step can always cache that information from the first time it runs with full compile.

My plan for true per file transpilation was to transform ES6 modules to CJS(like) modules so that a missing module is a runtime error. Obviously we cannot detect it correctly if you only give us a single file. And yeah to support mutable exports there at all we need Object.defineProperty.

Basically per file transpilation shouldn't ever ever assume things. That's bad.

[ChadKillingsworth]

I'm still not clear how your example function gets around this. You're importing in your example. imo @param {Foo} and @param {!/path/to/es6} don't really make sense.

Oh that was just an illustration of the two different places we would need to recognize and rewrite based off then imported module type. No I usually wouldn't mix the same import like that in the same file - but it definitely could happen (and works today).

I don't like treating modules as types that can be passed around tbh.

When I commonJS module imports another commonJS module, that path I listed is correct. But when a CommonJS module imports an ES6, there should be an import property listed.

My plan for true per file transpilation was to transform ES6 modules to CJS(like) modules so that a missing module is a runtime error. Obviously we cannot detect it correctly if you only give us a single file. And yeah to support mutable exports there at all we need Object.defineProperty.

I don't think that's good enough if you don't want to assume things. For per-file transpilation across modules, it requires a runtime check of the module type. What property an import references is dependent on both the host and target module types.

And I don't use per file transpilation and would love it if that use case wasn't supported. But as long as it is I figured I'd raise the issues that have to be addressed.

[jplaisted]

I don't think that's good enough if you don't want to assume things. For per-file transpilation across modules, it requires a runtime check of the module type. What property an import references is dependent on both the host and target module types.

Right, and because you know what was an Es6 module input you can add the __esModule property to it. The same way babel and other CJS transformers do things.

Anyways I think this may need a lot larger discussion here since I realizing how module rewriting is done in the compiler today is a huge blocker for this in general. Ordering sucks because every rewrite module step renames module scoped variables. So as it stands today you cannot actually move rewriting of references to module type into rewriter for module type because if it runs after some other module rewriter has run the local names are all already globalized.

And I don't use per file transpilation and would love it if that use case wasn't supported. But as long as it is I figured I'd raise the issues that have to be addressed.

Same here, at least with regards to ClosureBundler. Hotswap passes are not going away. But as it stands today CJS isn't compatible with ClosureBundler so it doesn't need to worry about it. It isn't compatible with ES6 modules either and I want to add support for them for internal people. Ideally it goes way but adding support is probably faster than killing it. But as it stands I think a lot of the code is making assumptions it doesn't need to.

[ChadKillingsworth]

There is definitely a lot involved here with often competing concerns: module interop, type checking, dead code elimination, per-file transpilation, whole world optimization, etc.

Whatever direction we go - we need a clear path charted that avoids one huge pr. The CommonJS pass is huge and complicated.

[jplaisted]

Okay, so I think we've managed to find a quasi happy compromise for now. I've updated the document I wrote.

The new tl;dr is:

• Non-mutated exports work the same way all exports work today since they aren't broken
• Mutated exports now become ES5 getters. Because the type checkers do not play well with this we will annotate them as unknown. This means that mutated exports will not be type checked.
• To the above two points: We will specifically check if an export is mutated or not. We won't assume that if it is mutable then it is mutated.

Additionally we should be able to support export * from while we're here as well as perform some more checks.

[jplaisted]

Going to drop export * from until later. We can get the basic functionality of mutable exports without preparsing every ES6 module.