[RFC] Re-write the module transpiler with support for bindings.

[eventualbuddha]

@wycats @thomasboyt @joliss @guybedford @stefanpenner @trek

This is an RFC for a reimplementation of the module transpiler that supports cycles and bindings. The architecture is pluggable so that custom formatters and resolvers can be provided. There are two types of bundled formatters in this rewrite: CommonJS, which is a 1-1 file mapping, and the bundled formats, which take N files and output a single bundle, similar to browserify. These are called "module-variable" and "export-variable".

This commit removes support for AMD, YUI, and globals formats. These formats can be implemented as plugins. AMD should perhaps be included in core - I don't really know.

There are still a bunch of things to do:

• Update README.
• Update CHANGELOG.
• Update TRANSITION.
• Should the bundle formats be included in core? Or are they a separate project? (core, for now)
• Add any missing tests from the original implementation.
• Decide what to do about node interop (__esModule flag #86) (nothing, for now)

[rwaldron]

These are all enumerable: false by default

[eventualbuddha]

Thanks. I'll change that. Larger question: is this a bad idea/pattern? Especially the memo helper?

[wycats]

I think AMD should be implemented in core, and we should make sure that uses of the other formats have a transition path other than "you're welcome to reimplement this yourself as a plugin".

[rwaldron]

I'm curious to know more about the decision to rewrite this in ES5?

[eventualbuddha]

@rwaldron partly to simplify the development of it, partly because I just hadn't gotten around to it. I did try dogfooding it, and may revive that, but this was very experimental for a while and I changed the structure and API a lot and didn't want to be slowed down. Now that it's a bit more stable I may revisit.

[eventualbuddha]

Here's an example of how you'd compile RSVP to CommonJS:

$ compile-modules convert -f commonjs lib/**/*.js -o dist/commonjs

And here's bundling into a file for the browser, given the new file below:

$ compile-modules convert -Ilib -f export-variable -o dist/rsvp.umd.js rsvp.umd

UPDATE 6/12: The "export-variable" formatter has been renamed to "bundle".

// rsvp/lib/rsvp.umd.js
import {
  Promise,
  EventTarget,
  all,
  allSettled,
  race,
  hash,
  hashSettled,
  rethrow,
  defer,
  denodeify,
  configure,
  on,
  off,
  resolve,
  reject,
  async,
  map,
  filter
} from './rsvp';

var RSVP = {
  Promise: Promise,
  EventTarget: EventTarget,
  all: all,
  allSettled: allSettled,
  race: race,
  hash: hash,
  hashSettled: hashSettled,
  rethrow: rethrow,
  defer: defer,
  denodeify: denodeify,
  configure: configure,
  on: on,
  off: off,
  resolve: resolve,
  reject: reject,
  async: async,
  map: map,
  filter: filter
};

if (typeof define === 'function' && define.amd) {
  define(function() { return RSVP; });
} else if (typeof module !== 'undefined' && module.exports) {
  module.exports = RSVP;
} else if (typeof window !== 'undefined') {
  window.RSVP = RSVP;
}

[guybedford]

The output is neat and seems to make a lot of sense for bundles. I've only looked through briefly, but have been unable to work out quite what the 1-1 output means.

For example, the following code:

import { p } from 'q';
export var q = 4;

With this command: ./bin/compile-modules convert -f commonjs test.js -o out.js

Outputs:

"use strict";
var q = 4;

Object.seal(Object.defineProperties(exports, {
  q: {
    get: function() {
      return q;
    },

    enumerable: true
  }
}));

Shouldn't the import be a require?

[eventualbuddha]

@guybedford That is definitely a problem. The assumption I'm making right now is that any import or export sources are themselves ES6 modules. If you need to pull in something which is not an ES6 module then you need to make a shim. This is perhaps untenable. Thoughts?

I updated this branch to throw an exception when it can't find a referenced module source.

[eventualbuddha]

@ebryn @wycats I pushed a branch with a custom resolver for Ember to show how you can bundle it using this re-written version of the transpiler. You can run something like the command listed here.

Note that this will build Ember, but that built bundle will not work. I worked around the first issue I found (bare Ember = {} in a use strict context), but there are others. I believe there's at least one place that a dynamic requireModule is being used, for example.

[eventualbuddha]

@dherman This is what I've been wanting to chat about, btw.

[caridy]

@eventualbuddha it seems that the plugin infrastructure is based on require() to get access to a custom class, which normally requires Rewriter class (e.g.: this is the plugin we are working on https://gist.github.com/caridy/b93c7db193ec7e6944fc#file-systemregisterrewriter-js-L5). I wonder if we should abstract out the rewriter infrastructure into its own pkg, and get the transpiler to use that for the default formats. If that makes sense, I can help with that, and eventually we can replace the guts of this pkg.

[eventualbuddha]

@caridy so you'd want to make the rewriter included by this project pluggable, similar to how the formatter stuff is? What is the rewriter you linked to intended to be used for?

[eventualbuddha]

Ping @domenic & @joliss for any feedback on spec compliance. If either of you could suggest any tests that we should add that'd be great (see test/examples).

[caridy]

@eventualbuddha no, I want rewriter class to be on its own repo so plugins (in a form of npm pkgs) can depend on that new pkg. Imagine the scenario where I have a system-register-rewriter extending the rewriter class to provide transpilation into System.register() format.

As for the example on the gist, it is producing a ES5 module format that preserves the semantics of ES6 modules, instead of relying on YUI or AMD formats. Note: the example is a variation of the traceur's implementation for modules.

[eventualbuddha]

@caridy Which Rewriter class are we referring to? The one on this branch, or something else?

[caridy]

yes, lib/rewriter.js, which is almost identical to the previous recast branch.

[caridy]

@eventualbuddha from the specs point of view, examples seem to cover all cases. From the test point of view, I think we should add more one example:

export default function hi() {
  return 'hi';
}
assert.equal(hi(), 'hi');

this will cover two cases, the case where the declaration name and the declaration init name are different, and the hoisting of the function that is exported as default.

[caridy]

There is an issue here, when you have export default function foo() {}. This is equivalent to a function declaration and the corresponding export. The transpiler should check if default is a function and it is named, then use b.functionDeclaration() to return two elements in the body of the replacement.

[eventualbuddha]

Are you sure? The spec says that export default is followed by an AssignmentExpression, which would make function foo() {} a function expression, not a function declaration. Am I misreading the spec?

[domenic]

Question: does this correctly prevent writing to imported bindings? I.e.

import { x } from "y";
x = 10;

should fail.

I don't remember if this is an early error (which should be caught at compile time), or if it's a runtime error. @dherman?

[eventualbuddha]

@domenic It depends on the formatter used. For the "bundle" formatter it will re-assign the original variable. For "commonjs" it will be a runtime error: TypeError: Cannot set property value of #<Object> which has only a getter.

Per the spec it seems that it should be an early, static error, assuming I'm reading this correctly:

It is a Syntax Error if LeftHandSideExpression is an IdentifierReference that can be statically determined to always resolve to a declarative environment record binding and the resolved binding is an immutable binding.

Import bindings are immutable, so this applies I think? I can make this a compile-time error.

[stefanpenner]

@eventualbuddha +1 handling this constraint compile time. Regardless what the spec suggests, for the bundle formatted i can't imagine implementing it sanely/performantly at runtime.

[eventualbuddha]

@domenic / @stefanpenner 7df865c adds this check at compile time.

[stefanpenner]

@eventualbuddha nice

[trek]

I've been short on time, so haven't been chiming in, but I ❤️ you all.

[domenic]

IMO this should be released ASAP, with iteration on it happening later. The only blocker is figuring out what to do regarding the different bundle formats. My preference order is probably:

• Include zero with core, to make people aware they need to choose one.
• Include only the complete-bundle format in core as a good default, with CJS/AMD/YUI as separate projects
• Include complete-bundle + CJS in core, because I like CJS more than AMD ;)

[caridy]

+1 as @domenic said, let's get this ship ASAP and iterate.

[ericf]

In the last TC39 meeting (last week) we had a Modules Breakout session to discuss many of the things @eventualbuddha, @guybedford, and @caridy have been working on for this update to the transpiler. Here's the notes from that session:

https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-06/jun-5-modules.md

[caridy]

@eventualbuddha I noticed that if there is an error in the formatter (custom formatter), the error is confusing, in fact, no error at all:

$ compile-modules convert -f system tests/**/*.js -o dist/
compile-modules [--help] [--version] <command> [<args>]

Commands

  convert  Converts modules from `import`/`export` to an ES5 equivalent.
  help     Display help for a given command.

A more descriptive error will be awesome.

[caridy]

@eventualbuddha are you planning to add support for classes as part of this PR? e.g.:

export default class foo {};
export class bar {};

[eventualbuddha]

@caridy Is that when loading the formatter or when actually processing? I ran it on one of the samples and got an error message:

/bin/compile-modules convert -I test/examples/duplicate-import-fails -f bundle importer

/Users/donovan/Development/es6-module-transpiler/bin/compile-modules:82
    throw ex;
          ^
SyntaxError: expected one declaration for `a`, at importer:7:14 but found 2
    at ImportDeclarationList.ModuleBindingList.findDeclarationForReference (/Users/donovan/Development/es6-module-transpiler/lib/module_binding_list.js:168:11)
    at ImportDeclarationList.ModuleBindingList.findSpecifierForReference (/Users/donovan/Development/es6-module-transpiler/lib/module_binding_list.js:130:26)
    at BundleFormatter.importedReference (/Users/donovan/Development/es6-module-transpiler/lib/formatters/bundle_formatter.js:236:31)
    at Rewriter.getExportReferenceForReference (/Users/donovan/Development/es6-module-transpiler/lib/rewriter.js:198:20)
    at Rewriter.processNodePath (/Users/donovan/Development/es6-module-transpiler/lib/rewriter.js:102:28)
    at NodePath.<anonymous> (/Users/donovan/Development/es6-module-transpiler/lib/rewriter.js:70:30)
    at NodePath.traverse (/Users/donovan/Development/es6-module-transpiler/node_modules/recast/node_modules/ast-types/lib/traverse.js:35:26)
    at NodePath.Pp.each (/Users/donovan/Development/es6-module-transpiler/node_modules/recast/node_modules/ast-types/lib/path.js:89:22)
    at traverse (/Users/donovan/Development/es6-module-transpiler/node_modules/recast/node_modules/ast-types/lib/traverse.js:30:18)
    at /Users/donovan/Development/es6-module-transpiler/node_modules/recast/node_modules/ast-types/lib/traverse.js:48:13

As for classes, I think running the esnext/es6-class transformer first will make it work.

[caridy]

@eventualbuddha a typo in the formatter is what I ran into while developing the formatter. Maybe the try/catch around the require() can get a more detailed error when the file exists but fails to evaluate.

[caridy]

this is way too strict. Ideally, we could check if the module can be resolved locally, but if it is not marked as relative (starting with .), we should issue a warning instead of throwing an error, while explicit relative modules should throw. This will cover things like:

import foo from "./path/to/foo";
import bar from "../path/to/bar";
import React from "react"; // this is not relative, but a top level module

in this case, saying that "react" is a non-verified dependency thru a warning should be just enough.

/cc @ericf

[ericf]

Module names are strings. Putting any emphasis on interpreting these names as a path seems like the wrong idea. I don't like this proposal, instead I think this should be handled at the resolver level by returning some type of "ignored" or "external" module obj.

Then the rest of the system could simply ignore validation when it encounters one of these "ignored" or "external" modules.

I could then create a resolver which has a known list of module names, e.g., "react", for which it returns an instance of a "ExternalModule".

[caridy]

My perspective on this is that we need to lower the complexity of using the transpiler, and for that, a warning is just enough so they can learn the basics and then peel out the layers for more advanced use cases like a more restrictive build.

[ericf]

@caridy @eventualbuddha I created a PR to demonstrate what I'm talking about here: #129

[caridy]

Along the same lines, we need to support compatFix for non-local modules. Saying, if I have import React from "react"; knowing that react is external, the output of the transpiler should be: var React = require(‘react’).default || require('react'); or something like that, this will allow us to interoperate with others module systems.