Add support for combined AMD+CommonJS module export format

[csnover]

The new module format outputs global-less modules that are compatible with both AMD and CJS loaders.

Fixes #2036.

[csnover]

So why is this a thing?

1. Right now it’s not very plausible to do IDE integration where saving a TypeScript file emits a usable cross-platform module; adding this combined module format is the easiest way to achieve it.
2. Wrapping after compilation is also not possible in all cases because outputting CJS and wrapping for AMD will omit the AMD-only dependencies, and outputting AMD and wrapping for CJS will cause modules to fail to load if any AMD-only dependencies were specified. This is somewhat of an edge case, but not as much as you might expect. (Wrapping after compilation will also break the source maps if they are not rewritten.)

[mhegazy]

thanks @csnover, we will need to talk about this in the design meeting. So I apologize in advance for the delay.

A few questions :

• why not UMD all the way. it does satisfy your requirements but also enable adding modules to the global namespace, which is used a lot, though not a good practice?
• why not use system.register? it seems that there are a lot of tools/frameworks that are supporting the ES6/ES7 module loader proposal, so would not that be sufficient?

[RichiCoder1]

Would agree with @mhegazy on both, especially on UMD.

[csnover]

why not UMD all the way. it does satisfy your requirements but also enable adding modules to the global namespace, which is used a lot, though not a good practice?

Doing this would require a valid JS identifier to be created for each module and I don’t know of any good way to do this automatically. One cannot just map module IDs by replacing "/" with "." and call it a day, for instance. Globals are an antipattern anyway for modular code so I’m not sure how useful it would be (though of course there is nothing that would preclude adding it later! :)). Long term, this module target (along with CommonJS and AMD targets) will hopefully become obsolete, though that is years away at least.

why not use system.register? it seems that there are a lot of tools/frameworks that are supporting the ES6/ES7 module loader proposal, so would not that be sufficient?

There is no ES6 module loader proposal, it was dropped from the ES spec and moved to WHATWG where it was significantly overhauled at least once. System.register is not part of the WHATWG spec any more and there is no platform that supports it natively. Outside of SystemJS I am unaware of any other tools that support any of these WHATWG APIs, but I haven’t looked very hard.

In any case it would be premature to try to use it as a target; we recently held a meeting for several different individuals with experience working on and developing module loaders for JavaScript (RequireJS team, Dojo team, CujoJS/RaveJS team, other stakeholders of the AMD specification), and the general consensus was that there are some design flaws that prevent the WHATWG proposal from being viable at the moment. There have historically been some issues getting the committee to engage on legitimate concerns that have been raised by community members with experience writing client-side loaders as well, and I don’t think that situation has improved any.

[mhegazy]

My main concern is introducing new concepts. ppl are familiar to some extent with what UMD is, i would hate to have something that is mostly UMD though not it, and then creating a new flavour.

Doing this would require a valid JS identifier to be created for each module and I don’t know of any good way to do this automatically. One cannot just map module IDs by replacing "/" with "." and call it a day, for instance.

I would think just the file name converted to an identifier, and error if not a valid one. e.g.: "foo.bar.ts" is an error, "foo.ts" becomes "foo". we have also added AMD module name support using ///<amd-module name='NamedModule'/>, and that would be your way out if you really want to use a "foo.bar.ts" and still meaningful UMD behavior in the global case.

[csnover]

My main concern is introducing new concepts. ppl are familiar to some extent with what UMD is, i would hate to have something that is mostly UMD though not it, and then creating a new flavour.

TS compiler already provides an option of outputting to CJS or AMD, this just combines the two, so it’s not really a new concept. It’s also not a new thing in the case of UMD, this code emits a (fixed) version of the “Node Adapter” pattern from the UMD repository, but since it doesn’t also emit any globals I didn’t want to imply that it did by calling it “umd”.

I would think just the file name converted to an identifier, and error if not a valid one. e.g.: "foo.bar.ts" is an error, "foo.ts" becomes "foo".

OK, so to make sure I understand what you thinking is here, what would these commands emit:

• tsc /path/to/file/foo/bar/baz.ts /path/to/file/foo.ts
• tsc /path/to/file/foo/foo.ts /path/to/otherfile/foo/foo.ts

And what would it look like for them to get references to other external modules they import? What if those external modules come from d.ts?

Probably more importantly, what is the point of doing all this extra work? How would you load these modules into a browser in a consumable manner without introducing an antipattern (manual dependency resolution)? I can tell you right now the benefit of this patch but I would have a hard time telling you what the benefit would be of making it also try to generate objects in the global scope and resolve all the edge case problems of trying to turn the global scope into a module registry.

we have also added AMD module name support using ///, and that would be your way out if you really want to use a "foo.bar.ts" and still meaningful UMD behavior in the global case.

AMD module IDs are module IDs, not JavaScript identifiers so I don’t think this is a way out either.

[mhegazy]

fair enough. thanks for the replies. i just find "camd" a bit unclear.. so maybe we just call it "umd" even without the globals. just an idea,

[csnover]

Naming is always the hardest part. It could be called “legacy” or “combined” or something too; I’ll take a quick poll at work and see if anyone has a preference, also cc team @umdjs for any thoughts on what might be a decent short name for an export that is a Node.js+AMD module without globals.

[csnover]

The serious alternative suggestions I received were:

• “es5” (supports the de facto standard module formats used by ES5 code)
• “adaptive” (following the name “Node Adapter”)
• “umd-m” (“UMD-Modular”)
• “lmd” (loader/legacy module definition)

Any preferences?

[mhegazy]

let me do a similar poll around here and see what are the preferences.

[basarat]

Isn't there any variation that is exactly the same as this: https://github.com/umdjs/umd

Perhaps there should be. 👍 for adding this.

[csnover]

Isn't there any variation that is exactly the same as this: https://github.com/umdjs/umd

See the above discussion :) or is there something that needs extra clarification?

[basarat]

That discussion seems focused on why we are not doing global but I don't think all "valid" umd variations need globals e.g this one : https://github.com/umdjs/umd/blob/master/nodeAdapter.js (am I wrong ?)

So I think what you have is still almost valid umd and can be made an exact valid umd variation "nodeAdapter" solves the naming problem (no new names created maybe even call it umd then)

[basarat]

Or we can ask umd JS to make what you have here a valid variation.

[basarat]

Also wouldn't the nodeAdapter version be potentially better for users that do:

var foo = require("foo");
import bar = require("bar");

[csnover]

Or we can ask umd JS to make what you have here a valid variation.

Unfortunately, and unbeknownst to many, most of the patterns in the UMD repository are wrong/broken. I already had to fix one of them last year and I could probably go through and find issues with everything in that repo.

Specifically the nodeAdapter.js in UMD repository is broken in these ways:

1. Uses an incorrect incantation for checking define (needs to check for define.amd per spec but does not)
2. Unconditionally sets module.exports with the return value of the factory function which means any module that sets values on exports and does not return a value is broken.

I am sure if I submit a PR to umdjs to change nodeAdapter to match the output of this patch it will be accepted.

Also wouldn't the nodeAdapter version be potentially better for users that do: […]

Not sure what you are getting at here, could you clarify?

[basarat]

could you clarify

With nodeadaptor this whole thing:

var foo = require("foo");
import bar = require("bar");

would get insert into : https://github.com/umdjs/umd/blob/master/nodeAdapter.js#L13-L17 as

var foo = require("foo");
var bar = require("bar");

And then foo and bar are treated in the same way i.e. amd or commonjs. Based on this PR only bar would get the AMD/commonjs love and foo would fail at runtime on amd environments (e.g. requirejs will say modulenotloaded last I tested).

[csnover]

Yes, that’s true, but var foo = require("foo") is an antipattern since import is available. This implementation allows amd-dependency comments to work correctly without requiring more changes in the emitter. In practice I don’t think this would be a problem (and non-goal #3 probably applies).

[basarat]

is an antipattern since import is available

agreed ❤️

[basarat]

I definitely prefer what you have here

[DanielRosenwasser]

Here are some ideas we're considering so far for the name. Feel free to add a few.

• umd
• unified
• commonamd
• camd

---

For the sake of humor, we also considered calling it "One Module System"

• OneMS
• 1MS

[rbuckton]

I've been giving some thought to this myself. I had been looking at a general purpose header that would support CJS, AMD, and System.register:

import { x } from './y';
export function z() {}
console.log(x);

// beginning of template
(function (deps, factory) {
    function bind(require, exports) {
        var module = factory(function (k, v) { return exports[k] = v; });
        for (var i = 0; i < deps.length; ++i) { module.setters[i] = require(deps[i]); }
        return module.execute();
    }
    if (typeof define === "function" && define.amd) {
        define(["require", "exports"].concat(deps), bind);
    }
    else if (typeof module === "object" && typeof exports === "object") {
        module.exports = bind(require, exports) || exports;
    }
    else if (typeof System === "object" && typeof System.register === "function") {
        System.register(deps, factory);
    }
})
// end of template
(["./y"], function (export_1) {
    var y_1;
    function z() {}
    export_1('z', z);
    return { 
        setters: [
            function (v) { y_1 = v; }
        ], 
        execute: function () {
            console.log(y_1.x);
        }
    };
});

[rbuckton]

CJS/AMD only might be something like this:

(function (deps, factory) {
    if (typeof define === "function" && define.amd) {
        define(["require", "exports"].concat(deps), factory);
    }
    else if (typeof module === "object" && typeof exports === "object") {
        module.exports = factory(require, exports) || exports;
    }
})
(["./y"], function (require, exports) {
    var y_1 = require('./y');
    function z() {}
    exports.z = z;
    console.log(y_1.x);
});

[csnover]

@rbuckton Hey, could you point me to some concrete information showing that those System.* APIs are ever actually going to be implemented by browsers or server-side JS engine vendors? They were eliminated from the WHATWG-formerly-ES6 loader specification quite some time ago, but people keep bringing them up. I don’t understand what is going on, except I know that the platform does not need a third incompatible never-standard module loader API, and SystemJS consumes CJS and AMD modules already.

[mhegazy]

@csnover thanks again, and sorry for the delay. Can you refresh the pull request against latest from master.

My vote is for "umd" and if not then "unified"; we can add more details in the help message and in documentation about differences from the standard UMD. Just to avoid introducing new concepts and/or acronyms.

As for system.js, we had a discussion about it today in the design meeting; while I agree there are no indications that systemJS support will be picked up by engines, TC39 standardizing a module syntax and semantics with no loader or bundeler pipeline has created a vacuum. SystemJS is based on the initial loader proposal that accompanied the es6 modules and that gives it momentum. Whether it is going to evolve into the standard is yet to be seen; there is enough support in tooling and workflows today that warrants considering it, at least from typescript standpoint.

For the purposes of this change, I do not think we need to worry about systemJS; assuming umd expands its definition to contain systemJS nothing stops us from adding it later on.

[DanielRosenwasser]

👍 umd

[csnover]

Sounds good, I will un-bitrot this early next week and change the name and let you know when it is updated. Have a great weekend!

[basarat]

umd 👍 (excited)

[danquirk]

Seems strange to me to call the flag --umd if it's only sorta umd. Our --amd and --commonjs are not sorta AMD or CommonJS modulo some differences.

[csnover]

@danquirk don’t do it man

[csnover]

I updated per the feedback above and rebased against master. I improved the implementation so it should work with Browserify users and looks a little prettier in the output.

Note there seems to be something wrong with the baseline test compiler, the compiler directives in all of the tests/cases/compiler/es5-* cases seem to be getting ignored. This is not a problem with the es5-umd test case added in this patch but a larger bug in the test system that should be tracked and fixed somewhere else. I simply accepted the baseline output since it was correct for all the other cases and I don’t have time to dig into what’s broken with these specific cases.

[mhegazy]

Note there seems to be something wrong with the baseline test compiler, the compiler directives in all of the tests/cases/compiler/es5-* cases seem to be getting ignored.

I just changed tests\cases\compiler\es5-declarations-amd.ts and test framework caught it, what do i need to do to repro this behavior?

[csnover]

@mhegazy If you look at the baseline reference output you will see it’s not an AMD module, it’s been compiled like no module option was specified.

[mhegazy]

that is because it is not a "module". a module needs to have at least one top level export or import. We have talked in the past about making this case to be emitted as a module regardless, but that seemed like a very corner case any ways (i.e. a module that does not export or import anything).

[rbuckton]

👍

[csnover]

Oh, right, of course. Everything is fine then!

[mhegazy]

Thanks @csnover!

[csnover]

Thank you!

[basarat]

Yay!

[niemyjski]

woot! can we get a beta2 with just this change :)

[mhegazy]

@niemyjski working on it. will update the road-map once we have a plan.

[niemyjski]

I just had to do this manually and ran into some issues and had to hack around them: https://github.com/exceptionless/Exceptionless.JavaScript/blob/master/dist/exceptionless.es5.js#L1613-L1627 so this would be greatly welcomed!

[niemyjski]

I really wonder how this would work with tsproject (https://github.com/ToddThomson/tsproject) It currently does bundling for the new es6 module syntax and works great. @csnover, if we outputted amd for every export, would it keep adding items to the global export (like the existing module output)?

[csnover]

@niemyjski sorry, I don’t know how that project works, so I can’t really give you an answer. Every loader is a unique snowflake when it comes to how built layers are consumed, so it’s not something that I think something like tsproject can actually do correctly without changing how modules work (which I suspect is what it does, mutating modules to no longer be modules and sticking them in a closure).

[niemyjski]

@csnover Thanks for the explanation. I'll look into it more.