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

External module bundling #4754

Closed
wants to merge 17 commits into from

Conversation

weswigham
Copy link
Member

Implements #4434 loosely based on the feedback in the issue and some internal feedback.

Changes from the original proposal:

  • The internal loader is commonjs based
  • The internal loader intercepts all imports, attempts to resolve them within the bundle, then passes them off to the external loader (if available)

Things to do (aside from integrate feedback):

  • many many tests
  • consider es6 module compatible emit
  • improve systemjs support - we don't support full es6 semantics even when targeting systemjs with this loader, and we can't handle dynamic systemjs external dependencies (since they're async)
  • other internal loader options?

@@ -167,6 +245,64 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
/** Called once the emit of the node is done */
let emitEnd = function (node: Node) { };

/** Called to build the opening part of a bundled module wrapper */
let emitBundleWrapperStart: Map<Function> = {
Copy link
Member

Choose a reason for hiding this comment

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

This should be Map<() => void>.

write(`System.register([], function(${exportFunctionForFile}) {`);
writeLine();
increaseIndent();
let exportStarName = emitExportStarFunction(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

/* localNames */

switch(part) {
case "":
case ".":
break;
Copy link
Member

Choose a reason for hiding this comment

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

Indent from case clauses

@gulbanana
Copy link

does this implementation support the use case of transitive multi-file bundles?

scenario:
typescript project A contains Class1 and Class2. Class2 depends upon NpmModule
typescript project B contains Class3, depending on Class1 or Class2.

the structure we really want is a namespace for each of A and B and something 'dll-like' which wraps them up. however, Class2 must be defined in an external module to be able to import NpmModule. this causes a cascade of module usage:
Class1 must be an external module so that it can be bundled with Class2 into bundle A. Class3 must now be an external module so that it can import A, and from A, Class1 or Class2.

in reality project A and B might each have dozens or hundreds of classes, none of which have any reason to be defined in modules and internally microloaded other than the fact that some leaf of the internal dependency tree uses NpmModule. is this going to function without causing some sort of massive overhead?

@weswigham
Copy link
Member Author

In the end, this at present does support offloading module dependencies to the external module loader when in-bundle resolution fails (the bundle consisting of every TS file in your project, rather than just those deemed 'reachable' from the entrypoint). This passthru works well for commonjs and even okayish for amd, but for systemjs it doesn't work particularly well - in fact, in system it presently doesn't deal with external/non-ts dependencies well at all.

On that note - should our bundler bundle in reachable non-ts dependencies, a la browserify or webpack? Or is the TS project the exclusive scope of the bundle, and external bits are left to the user to package/bundle? A practical example would be bundling angular2 - I've been able to bundle it with the bundler here with some changes without it bundling in Rx, zone.js, or reflect-metadata and it builds (and runs)... but there is a call to require('rx') that at present goes unfulfilled unless the loader using the bundle provides it. (And reflect-metadata needs to get included in the page, since it patches the global Reflect object.)

In either case, in situations like systemjs or amd style dependency emit, it would be useful to be able to specify external dependencies in a hash of internal name to external name (this isn't useful for commonjs emit since we can just yield to the platform require):

"bundleDependencies": {
  "angular2/angular2": "angular2",
  "jQuery": "jquery"
}

to allow your bundle-external import statements

import * as ng from 'angular2/angular2';
import * as $ from 'jQuery';
import SomeComponent from './somecomponent';
ng.bootstrap(SomeComponent);
$('title').html('SomeComponent Test');

to be depended upon correctly in the resulting bundle:

System.register(["angular2", "jquery"], function(exports_1) {
  var angular2_min_js;
  var jquery_min_js;
  //...
  return {
    setters: [function(m){angular2_min_js = m;}, function(m){jquery_min_js = m;}],
    execute: function() {
      //...
    }
  };
});

I propose adding this option hash, rather than traversing imports and attempting to identify what is bundle local ourselves, to support the same dynamic cases as mentioned in the original issue, but with bundle-external modules.

Granted, we could do both - attempt automagic external dependency finding and defer to such a hash option if present.

@weswigham
Copy link
Member Author

After some conversation, I think we've decided to simply autodetect external imports - I'm considering confirmed external imports as external and undefined imports (imports TS couldn't find) external.

It also looks like we're going to just bundle TS, and only TS within our own package - so I'll need to test that we don't bundle anything across package bounds.

@DanielRosenwasser
Copy link
Member

Can you include work as part of #1544 in this change so that we issue an error and quit when

  1. --out or --outFile are used and
  2. Either
    1. --module is specified
    2. You are compiling in ES6 mode and any of your files has an external module indicator.

Or some variation of the above.

@weswigham
Copy link
Member Author

@DanielRosenwasser You sure you didn't want to tack that on to #4811 instead? It deals with changes to --module already. (And TBH I've been waiting on it getting merged to implement ES6 module bundle emit.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants