Full System.register AMD wrapping

guybedford commented 9 years ago

It would be possible to make AMD a full wrapped format when bundled like the others as System.register by applying transforms similar to the global transforms we have for handling globally-scoped assignments.

A fully-wrapped System.register output would simplify the meaning of an AMD bundle and unify the ability to subtract bundles, which currently needs some exceptions to work.

Rush commented 9 years ago

Maybe it's related ... update from systemjs/builder from 0.13.x to 0.14.5 broke import of AMD modules using System.import("application.js"); .. I had to load it from System.amdRequire(["application.js"]). I understand that if this issue was fixed, I could again use System.import.

guybedford commented 9 years ago

@Rush that sounds like something else. System.amdRequire just calls System.import so I'm not sure how that would have fixed it.

guybedford commented 8 years ago

Full explanation:

Right now, AMD is replaced in-place:

function global() {
}

var anotherGlobal = 5;

// yes we allow named defines alongside the define below
define('x', function() {
});

if (thing)
  define(function() {
    return { a: global, b: anotherGlobal };
  });

Where we just replace the define statement itself with System.registerDynamic('...', ...) to ensure the exact same execution behaviour. The existing named define gets left as-is.

The issue with this is that AMD is then not a "wrapped" format... it still runs with its expected bindings. So we're not doing strict conversion like we do with the others where we have a reliable form of:

System.registerDynamic(..., function() {
  // all code is wrapped in here
});

In order to turn AMD into a wrapped format, this involves doing the same thing we do for global transformers by assigning variables to the bindings:

System.registerDynamic('y', ..., function(require, exports, module) {
  // a standard prefix?
  var define_value;

  function global() {
  }

  var anotherGlobal = 5;

  // named define gets converted into a nested System.registerDynamic!?
  System.registerDynamic('x', [], false, function() {
    // ...
  });

  if (thing)
    // we convert the define into an IIFE
    define_value = (function() {
      // exports and require in scope need to be handled
      return { a: global, b: anotherGlobal };
    })();

  // update global assignments back to global
  this.global = global;
  this.anotherGlobal = anotherGlobal;

  // return
  return define_value;
});

The other benefit of this would be that we no longer need to rely on the global define being present as well.

This is quite complex because AMD has so many frickin forms... but I think it would be a nice opportunity to start.

guybedford commented 8 years ago

Unfortunately I think the nested AMD defines cause an issue here. Specifically for a case like:

define(['c'], function(c) {
  return c;
});

define('c', ['b'], function(b) {
  return {
    b: b,
    c: 'c'
  };
});

If we nest the second named define like I've done in the example above, then we don't get the definition made at the right time as it has to happen along with the execution of the module.

We could choose not to support this case, but it has proven useful for AMD cases.

So I think we should put this direction on hold for now. While it seems nice conceptually, the benefits aren't that clear as well.

jrauschenbusch commented 8 years ago

Is it necessary to nest the System.registerDynamic calls? Or could it be possible to call them sequentially, resulting in multiple sequential System.registerDynamic calls in one file? In this case the execution would happen along the execution of the module.

guybedford commented 8 years ago

The issue comes where you have cases like:

(function() {
  // named define
  define('name', ...);

  // anon define
  define([], function() {
  });
})(typeof self != 'undefined' ? self : global);

Because of the fact that the named define is wrapped up inside the same closure as the anonymous define, it's hard to wrap up the anonymous define, while separating the named define.

If we always just had the case:

define('named', ...);

define([], function() {});

Then we would be ok though.

Perhaps this case could be separated though by ONLY allowing the above "top-level" named defines to be supported when applying this process.

jrauschenbusch commented 8 years ago

Ok, so you think the problem could be the scope of a closure? What if we leave such a closure untouched and only wrap the internal defines?

(function() {
  // named define
  System.registerDynamic('named', [], function() {
  });

  // anon define
  System.registerDynamic('anon', [], function() {
  });
})(typeof self != 'undefined' ? self : global);

jrauschenbusch commented 8 years ago

Ok, i see. It's again no full wrapping. :-(

guybedford commented 8 years ago

Ok, so we could do this, but then the only benefit is that we're using System.registerDynamic instead of having to rely on the define being present. This is still a benefit over requiring the global define to be present though as that is an environment-altering constraint which is not ideal.

So if we could do just the above that would be good.

The complexity comes in with the AMD edge cases though - cases like ensuring require and exports are handled correctly in requires etc etc. Handling those edge cases would be some work, but would be possible.

I've finally put together the test cases at https://github.com/guybedford/system-register-dynamic-amd-cases that demonstrate how this might work for the various cases. The only case that seems like it may not be possible to work with is handling the contextual require of define(function(require) { require(['x']); }), but I'm not sure how widely that is used. But System.register will soon properly support proper contextual loaders in general as soon as that spec comes out (likely at the next TC39 meeting in two months), so we may then have options for remapping then based on static analysis of these cases.

The alternative remains just to do the identical replacement, perhaps with the addition of replacing define statements with System.amdDefine so we no longer rely on the global define being present, which is still a win. I do think the future of these transformers though has to lie in handling everything at the transformation layer though as that will provide the best simplification and performance I think ideally. So the question then is just whether we do that now or later.

@jrauschenbusch just let me know if you have any further questions at all (when you're back from that holiday of course!).

jrauschenbusch commented 8 years ago

Back from vacation i would also say that the right way is to handle as much as possible in the transformation layer. Hence i would start with the new approach based on your test cases.

One question: What is the reason that handling a contextual require constitutes a problem?

I've found the following related proposal (nested imports), but it's still in stage 0: https://github.com/benjamn/reify/blob/master/PROPOSAL.md

Did you meant this spec as foundation for the contextual loaders in SystemJS?

The question is how likely/fast the spec will come out, because there are still several discussions about the syntax of nested imports: https://github.com/tc39/ecma262/pull/646

guybedford commented 8 years ago

@jrauschenbusch from reading the last TC39 meeting notes, it looks like there was consensus to iterate on the contextual loader spec to look something like:

var m = await import('x')

With another separate contextual metadata spec being worked on along the lines of:

var currentModuleKeyInRegistry = import.key;

These specifications should be presented very soon at either the coming TC39 meeting or the following one. As soon as those are out, we can work on the Babel transforms and System.register support to provide this as a first-class feature in ES modules.

Once contextual imports are given first-class support in the builder, we open lots of new doors around automatic chunking like webpack and possibly supporting AMD dynamic require treating it as a proxy to this.

jrauschenbusch commented 8 years ago

Ok. Should we still start with the present test cases and later adopt the contextual loader spec or should we wait?

I've created a inital repository for the amd transformer plugin with all your created test cases in place: https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper

guybedford commented 8 years ago

Looks great! I think we'd be good to start moving, the contextual stuff will come later.

jrauschenbusch commented 8 years ago

Allright. I've started with making the tests pass. Since there are a lot of tests cases it could take a while.

jrauschenbusch commented 8 years ago

Hi @guybedford,

a first - but certainly not perfect - implementation is now ready for initial tests. All defined unit tests are passing.

@asapach Maybe you could help me with the code optimizations/bug fixes?

guybedford commented 8 years ago

@jrauschenbusch this looks amazing! Do you think you could send a PR to the builder project with this? I can then run it against the builder and jspm test cases and see if anything comes up there too.

guybedford commented 8 years ago

@jrauschenbusch some initial feedback:

The check in https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper/blob/master/src/index.js#L40 is doing extra work as it's looping up from each call down the tree. It should be possible to store a tree-level piece of state (on opts or otherwise, not sure what the best practice is here in Babel?) that checks directly in the CallExpression visitor if this call expression is a "System.registerDynamic" call expression, and then notes via a state flag that we're inside that call expression for the subsequent visitor calls until we leave it again.
As from the earlier comment, the path at https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper/blob/master/src/index.js#L230 is an unnecessary branch - it should always be typeof factory == 'function' first which can probably be done directly in the template at https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper/blob/master/src/index.js#L18.
In the define.amd visitors, include the check at line https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper/blob/master/src/index.js#L264 as the first item in the conditional check before doing the rest like path.scope.hasBinding because that will provide better performance filtering out the unnecessary checks earlier.
The same point applies to https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper/blob/master/src/index.js#L54

jrauschenbusch commented 8 years ago

@guybedford: Thanks for the feedback. Points 2-4 are fixed. Regarding the nested define handling maybe @asapach can provide a better solution. There's certainly a better solution.

I've manually installed the plugin and validated it against the builder test cases. Some test cases break now. I've created a separate branch in my builder repo. PR will follow in a few minutes, but we've to fix the test related issues before a final merge can be applied.

asapach commented 8 years ago

One of the suggestion I came up with for (1) is to separate the visitor into enter/exit pair an track the state that way, e.g. if we enter System.registerDynamic() we ignore the transform until the matching exit.

jrauschenbusch commented 8 years ago

@asapach Sounds good. Incidentally, any PR to https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper is very welcome!

jrauschenbusch commented 8 years ago

@guybedford Currently i've no clue how to fix the issues mentioned in https://github.com/systemjs/builder/pull/680. The initial loading problem seems to be related to the output of the tree-build.js file:

Error loading resource /builder/test/output/global.js (203). Details: Error opening /builder/test/output/global.js: No such file or directory
Error loading resource /builder/test/output/plugin.js (203). Details: Error opening /builder/test/output/plugin.js: No such file or directory
Error loading resource /builder/test/output/text-plugin.js (203). Details: Error opening /builder/test/output/text-plugin.js: No such file or directory
...

The others seems to be related with the output format of the modules itself:

Module /builder/test/output/amd-4.js interpreted as global module format, but called System.register.
Module /builder/test/output/amd-7.js interpreted as global module format, but called System.register.
Multiple anonymous System.register calls in module /builder/test/output/amd-11.js. If loading a bundle, ensure all the System.register calls are named.
Potentially unhandled rejection [46] Error: (SystemJS) Module /builder/test/output/umd.js interpreted as cjs module format, but called System.register. Error: Module /builder/test/output/umd.js interpreted as cjs module format, but called System.register.

jrauschenbusch commented 8 years ago

Ok. The first issue is now detected. Required paths must be mapped from $__require('./seconds.js') to $__require('seconds.js').

jrauschenbusch commented 8 years ago

The »Module x interpreted as global module format, but called System.register« issue can be solved by adding the following to the SystemJS config in /builder/test/test-build.html:

System.config({
   ...
   meta: {
      'output/*': { format: 'register' }
   },
   ...
});

jrauschenbusch commented 8 years ago

The »Multiple anonymous System.register calls in module /builder/test/output/amd-11.js. If loading a bundle, ensure all the System.register calls are named.« issue seems to caused by the concatenation of the builder output of /fixtures/test-tree/amd-10.js and fixtures/test-tree/amd-11.js in output/amd-11.js which results in multiple anonymous System.register calls.

output/amd-11.js:

"bundle";
System.registerDynamic('a', [], false, function ($__require, $__exports, $__module) {
  return {
    a: 'a'
  };
});

System.registerDynamic('b', [], false, function ($__require, $__exports, $__module) {
  return {
    b: 'b'
  };
});

System.registerDynamic(['c'], false, function ($__require, $__exports, $__module) {
  return (function (c) {
    return c;
  }).call(this, $__require('c'));
});

System.registerDynamic('c', ['b'], false, function ($__require, $__exports, $__module) {
  return (function (b) {
    return {
      b: b,
      c: 'c'
    };
  }).call(this, $__require('b'));
});
(function () {
  var define = System.amdDefine;
  define('a', { a: 'a' });
  define('b', { b: 'b' });
  define("amd-10.js", ["c"], function (c) {
    return c;
  });
  define('c', ['b'], function (b) {
    return {
      b: b,
      c: 'c'
    };
  });
})();

System.registerDynamic(['amd-10.js'], false, function ($__require, $__exports, $__module) {
  return (function (m) {
    return m;
  }).call(this, $__require('amd-10.js'));
});

guybedford commented 8 years ago

@jrauschenbusch the fix should be straightforward - the issue is the way in which the builder short-circuits the dependency analysis to avoid code execution, which involves attaching the dependency parser into the instantiate loader hook in the attach method.

In order to make this work, we do need another transformer though - a very simple highly-optimized getAMDDeps function. Effectively like that piece of Traceur code we currently have. But having this rewritten will provide the ability to then fully remove Traceur and use a single parse tree.

I'm happy to do the final integration, or even look at the simplified getAMDDeps transformer (which is just a visitor not a transformer really to return the array of dependencies).

Just let me know if you have any questions further.

jrauschenbusch commented 8 years ago

Hi @guybedford,

as you predicted it the rewrite of the getAMDDeps function does the trick. Currently it is not very optimized, but its still WIP.

Currently there are two more issues to solve: Inline source maps and sfx builds.

  1) Source Maps can render inline:
     Uncaught AssertionError: expected 1 to equal 0
      at /builder/test/sourcemaps.js:83:14
      at tryCatcher (/builder/node_modules/bluebird/js/release/util.js:16:23)
      at Promise._settlePromiseFromHandler (/builder/node_modules/bluebird/js/release/promise.js:510:31)
      at Promise._settlePromise (/builder/node_modules/bluebird/js/release/promise.js:567:18)
      at Promise._settlePromise0 (/builder/node_modules/bluebird/js/release/promise.js:612:10)
      at Promise._settlePromises (/builder/node_modules/bluebird/js/release/promise.js:691:18)
      at Async._drainQueue (/builder/node_modules/bluebird/js/release/async.js:138:16)
      at Async._drainQueues (/builder/node_modules/bluebird/js/release/async.js:148:10)
      at Immediate.Async.drainQueues [as _onImmediate] (/builder/node_modules/bluebird/js/release/async.js:17:14)

  2) Test tree builds - Babel SFX tree build:
     Error: Phantom test failed test/test-sfx.html failed.
      at Error (native)
      at ChildProcess.<anonymous> (/builder/test/test-build.js:24:16)
      at maybeClose (internal/child_process.js:821:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)

  3) Test tree builds - TypeScript SFX tree build:
     Error: Phantom test failed test/test-sfx.html failed.
      at Error (native)
      at ChildProcess.<anonymous> (/builder/test/test-build.js:24:16)
      at maybeClose (internal/child_process.js:821:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)

The decoded inline source maps looks like this:

{ version: 3,
  sources: [],
  names: [],
  mappings: '',
  file: 'output.js',
  sourcesContent: [] }

And the sfx tree builds say ReferenceError: Can't find variable: System.

Additionally i've a question regarding the remap function of the AMD transformer. In order to replace the traceur logic here we still need actually a real transformer and not just a visitor, or am i wrong?

jrauschenbusch commented 8 years ago

Steps to reproduce:

clone all repos
change builder to branch upgrade-amd-transformer (git checkout upgrade-amd-transformer)
run npm i in each project
run npm pack in each plugin folder
run npm i <path-to-plugin>/<plugin>.tgz in builder folder for the two plugins
run npm test

List of repos: https://github.com/jrauschenbusch/builder/tree/upgrade-amd-transformer https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-dependencies https://github.com/jrauschenbusch/babel-plugin-transform-amd-system-wrapper

jrauschenbusch commented 8 years ago

It seems that a missing transformation causes the ReferenceError: Can't find variable: System problem:

The file builder\test\output\sfx.js contains some System.registerDynamic(...) calls instead of $__System.registerDynamic(...).

By manually fixing it another error occurs: Uncaught Module 1 not present.

jrauschenbusch commented 8 years ago

Ok, i've found the source of the sfx issues. It seems that some amd modules should be transformed to named System.registerDynamic() calls, but are transformed to anonymous calls. Furthermore exactly this System.registerDynamic() calls aren't mapped to the $__System identifier which leads to the first issue.

Here is the diff output:

diff --git a/sfx-babel.js b/sfx-traceur.js
index 19efbde..26586ba 100644
--- a/sfx-babel.js
+++ b/sfx-traceur.js
@@ -125,7 +125,7 @@
                         return i(b.normalizedDeps[c]);
                 throw new TypeError("Module " + a + " not declared as a dependency.")
             }, c, d);
-            l && (d.exports = l),
+            void 0 !== typeof l && (d.exports = l),
             c = d.exports,
             c && c.__esModule ? b.esModule = c : b.esModule = k(c)
         }
@@ -454,15 +454,15 @@
         return c.exports = "This is some text",
         c.exports
     }),
-    System.registerDynamic(["3", "4", "5"], !1, function(a, b, c) {
-        return function(a, b, c) {
+    function() {
+        var a = $__System.amdDefine;
+        a("6", ["3", "4", "5"], function(a, b, c) {
             return {
                 is: "amd",
                 text: c
             }
-        }
-        .call(this, a("3"), a("4"), a("5"))
-    }),
+        })
+    }(),
     $__System.register("7", [], function(a) {
         "use strict";
         var b, c;
@@ -538,17 +538,16 @@
             }
         }
     }),
-    System.registerDynamic(["9", "b"], !1, function(a, b, c) {
-        return c.uri = c.id,
-        function(a, b, c, d) {
+    function() {
+        var a = $__System.amdDefine;
+        a("1", ["9", "b", "require", "module"], function(a, b, c, d) {
             d.exports = {
                 first: a,
                 second: c("b"),
                 utfChar: "∞"
             }
-        }
-        .call(this, a("9"), a("b"), a, c)
-    })
+        })
+    }()
 })(function(a) {
     amd1 = a($)
 });

guybedford commented 8 years ago

@jrauschenbusch for the System global, this should be read from the opts.systemGlobal compiler option (see https://github.com/systemjs/builder/blob/b58b464ed588c4695a2f077051eeb24c7b0fe1cd/compilers/register.js#L9).

Further, the named form should always apply to the primary anonymous define (there should only be one in any given file, the others are treated as secondary named defines) when the moduleName compiler option is set (https://github.com/systemjs/builder/blob/b58b464ed588c4695a2f077051eeb24c7b0fe1cd/compilers/register.js#L5), and not when not.

jrauschenbusch commented 8 years ago

Ok, these two issues are fixed now. But new problems have arisen.

The unit tests are broken for amd-6a and amd-8.

Output amd-6a:

"bundle";
System.registerDynamic('amd-6a.js', [], false, function ($__require, $__exports, $__module) {
  return (function (require) {
    this.p = 'a';
  }).call($__exports, $__require);
});

Output amd-8:

"bundle";
System.registerDynamic('amd-meta-dep.js', [], false, function ($__require, $__exports, $__module) {
  var _retrieveGlobal = System.get("@@global-helpers").prepareGlobal($__module.id, null, null);

  (function ($__global) {
    window.meta = 'dep';
  })(this);

  return _retrieveGlobal();
});
"deps ./amd-meta-dep.js";

if (!window.meta) System.registerDynamic("amd-8.js", [], false, function ($__require, $__exports, $__module) {
  return (function () {
    return window.meta;
  }).call(this);
});

And the source maps issue for inline source maps is still present. Oddly, because when i change the sourceMaps option from 'inline' to true the content of output.sourceMap looks good?!

jrauschenbusch commented 8 years ago

I've fixed the amd-6 and amd-8 issue. Maybe the amd-6 issue isn't correctly fixed, since i solely changed the expectation from a global property to an expected module property (https://github.com/jrauschenbusch/builder/blob/upgrade-amd-transformer/test/test-build.html#L151).

The inline sourcemap issue is still open. And with a current merge of systemjs/builder's master branch the sfx-bundle test is broken. The sfx-amd test is still working.

@guybedford Do you have any ideas?

guybedford commented 8 years ago

@jrauschenbusch thanks for the update. I just tried to checkout the branch there but can't run it because I need a built version of babel-plugin-transform-amd-system-dependencies to be present. Any chance you can publish this / or at least make the dist folder available on the tag or branch in github?

There does seem to be quite a lot of repeated code between babel-plugin-transform-amd-system-wrapper and babel-plugin-transform-amd-system-dependencies. It should be possible to use the same transform with some flexibility for both here I think, which may cut down on this.

jrauschenbusch commented 8 years ago

@guybedford Hi, i've pushed the built plugin npm packages to the repos (.tar.gz files at the root level). I've fixed the inline sourcemaps issue in the jrauschenbusch\build repo (upgrade-amd-transformer branch). You can install the .tar.gz packages directly outgoing from your builder root folder: npm install ../path/to/plugin/<plugin>.tar.gz (jrauschenbusch\builder -> upgrade-amd-transformer)

Currently I would publish the plugins merely reluctantly already, since they are still in a very early stage. Or what do you think?

The sfx issue was introduced by a merge of the systemjs\builder repo: https://github.com/jrauschenbusch/builder/commit/1d1a847a81f8c52bb70cdbd34c8cd7cb191eca05

Regarding the repeated code and the potential optimizations: It's quite clear that we need to consolidate the code and optimize it. This was just a first shot to understand, what we need a how we can solve it. For the optimizations I would look forward for support.

jrauschenbusch commented 8 years ago

@guybedford I've run some additional tests. If I reset the builder HEAD to https://github.com/jrauschenbusch/builder/commit/dd7fc6be1678a8303a140227de86b01325651b14 and afterwards cherry pick the commit https://github.com/jrauschenbusch/builder/commit/617c5e11fd080741afa666ecca295d88d5ae6e22 everythings works fine. Now I must still find the source of the problem (merge of systemjs\builder), which makes the sfx test fail..

jrauschenbusch commented 8 years ago

I've tracked down the problematic part in the sfx.js output. But I've currently no clue which modification of the merge is responsible for the changed output.

diff --git "a/.\\sfx-working.js" "b/.\\sfx-not-working.js"
index 4d03f68..8d1d819 100644
--- "a/.\\sfx-working.js"
+++ "b/.\\sfx-not-working.js"
@@ -114,7 +114,7 @@
                     if (b.deps[c] == a) return i(b.normalizedDeps[c]);
                 throw new TypeError("Module " + a + " not declared as a dependency.")
             }, c, d);
-            l && (d.exports = l), c = d.exports, c && c.__esModule ? b.esModule = c : b.esModule = k(c)
+            void 0 !== typeof l && (d.exports = l), c = d.exports, c && c.__esModule ? b.esModule = c : b.esModule = k(c)
         }
     }

UPDATE I have found the causing change: https://github.com/systemjs/builder/commit/6639c1b8d96ecf4bccbd0a275383820c058360e7. Using the old template everything works fine.

jrauschenbusch commented 8 years ago

Hi @guybedford,

I've consolidated the jrauschenbusch\builder commits in the upgrade-amd-transformer branch and merged the current HEAD of systemjs\builder into it. With the last commit each unit test will be passed. Now it's the time to think about the optimization and integration path. An open point would be the remap function, for which you would have to extend the wrapper plugin.

Please look at the necessary builder changes and give me a first feedback on what you think of it.

Best regards, Jochen

jrauschenbusch commented 7 years ago

@guybedford Can we also close this ticket due to the merge of #764?

Only the contextual loader thing is currently not adressed as you've mentioned here: https://github.com/systemjs/builder/issues/264#issuecomment-242099766

guybedford commented 7 years ago

@jrauschenbusch thanks, I will close this on release. Yes contextual import work in the builder ala webpack code splitting to do automated bundling is the next frontier in the builder stuffs.

I've just made some changes to the codebase of babel-plugin-transform-amd-system-wrapper to update to the SystemJS 0.20 registerDynamic form. Can you please add me as a collaborator so I can publish with the release for tomorrow? Hope you don't mind the code style changes too much too 😛

jrauschenbusch commented 7 years ago

@guybedford I've added you as owner to the npm package. Now you can publish updates. And yes, also code style changes are welcome. Mine is a bit influenced of my Java development.

guybedford commented 7 years ago

Implemented in 0.16 thanks to @jrauschenbusch.

systemjs / builder

Full System.register AMD wrapping #264