systemjs / builder

SystemJS build tool
MIT License
465 stars 122 forks source link

Bundling global dependencies issue. #5

Closed VasilioRuzanni closed 10 years ago

VasilioRuzanni commented 10 years ago

@guybedford

The system.js is super-convenient due to its ability to load any module and automatically resolve its format. Awesome.

When system.js loads different modules as different files which is pretty good for development - everything works perfect and no other configuration is even required aside from setting the paths for third-party dependencies. All things "just work".

There are problems though when trying to "bundle" everything for optimized production usage. Issue raised when moved the medium-sized Angular app to using ES6 with Traceur and ES6 Module Loader and tried to prepare the combined/concat-ted build file.

I would like to use SystemJS bundled in production too, so that I don't have to deal with AMD/etc at all. Calling System.import(...) works just fine for me and nowadays seems to be recommended approach according to your article (Dynamic Workflow 4).

Now, the issue. While in dev, if I refer to dependency like

import angular from 'angular';

// The `angular` is what it should be here, things work and everyone is happy.
angular.module(...);

When I use systemjs-builder to create a bundle though (my app imports the angular and that gets into the bundle too, which is OK), the result of import looks like

import angular from 'angular';

// angular == { angular: Object, ng339: 3 }

For me, it seems like systemjs-builder might have something like shim config missing (like we do for RequireJS). After intensive googling, no ways to provide any kind of shim config for build were found. Maybe there is some other way to configure that for builder? Or am I missing something?

So, basically the questions are:

Thanks in advance.

guybedford commented 10 years ago

SystemJS builder is designed to entirely match the behaviour of SystemJS. So if you are noticing any differences then that is a bug.

To clarify the issue - can you share your bundle output here?

VasilioRuzanni commented 10 years ago

@guybedford Sure. I'm trying to wrap my head around what exactly is happening there.

As for this particular Angular issue - seems like in dev it imports angular.js just fine but it seems that angular sets some internal ng339 property to global scope (needed for jqLite internal stuff) during the execution. This is indeed interesting, because SystemJS loads what it needs perfectly. But systemjs-builder makes an angular not the module itself but a property instead alongside that mysterious ng339.

Not sure how to post the result of systemjs-builder build with Angular here since its pretty lengthy even for Plunker. There is another Plunkr I created for you though where I just have few dependencies as example including that one imitating "global".

Take a look at this plunker. Open up the browser console and hit "Run" then look at the console messages.

Now, the odd thing (can be seen by looking at the console output):

Hope this explanation makes sense.

Feel free to ask for more detail and please confirm whether this is a buggy behavior or some hidden feature :)

I'll experiment a little bit on passing multiple "global" dependencies to see what happens.

guybedford commented 10 years ago

The issue sounds like you aren't loading the SystemJS configuration in production - which is still necessary after the build.

As for the System.register issue, I think I may have found a possible issue and posted a PR at https://github.com/systemjs/systemjs/pull/184.

guybedford commented 10 years ago

Sorry - including the config in production isn't strictly necessary to fix the global issue.

If you update to SystemJS builder 0.1.2 that should fix the Angular issue here.

VasilioRuzanni commented 10 years ago

@guybedford Well, things seem to get better a little bit but still there are some issues (unless things are extremely explicitly configured).

I might have missed some configuration or something so it would be awesome if you could point me out to something to look at.

Ok, now some things step-by-step: 1) I used the version of SystemJS from your PR here, the issue with one dependency being overwritten with another has completely disappeared which is awesome. Waiting for that to be merged and propagated up to npm.

2) I had to specify the System.bundles explicitly for angular to be what it should be. Example: When I try to just

System.import('angular');

// it fails because `angular == { angular: Object, ng339: 3 }`

but when I explicitly specify the bundle and the path:

System.paths['all'] = '/build/app-built.js'
System.bundles['all'] = ['app/app', 'angular'];

// then
System.import('angular');
// this works and angular is what it should be.

This kinda works but reminds of the RequireJS configuration hell a little bit. Moreover, if I don't specify this bundle config, the UMD-wrapped modules of my app are not being resolved correctly. I also have to explicitly add

window.define = System.amdDefine;
window.require = System.amdRequire;

for AMD (and I guess UMD-wrapped) modules to work too.

Basically put, the question is whether it is possible to just bypass this kind of bundle config and just load the app-built.js file which contains all the module definitions wrapped with System.register anyway and let systemjs-builder and SystemJS do all the heavy lifting like that:

<script src="traceur.js"></script>
<script src="es6-module-loader.js"></script>
<script src="system.js"></script>
<script src="app-built.js"></script>
<script>
  // All the modules imported here are in the 'app-built.js' file
  System.import('app/app');
  // or
  System.import('angular');
</script>

instead of:

<script src="traceur.js"></script>
<script src="es6-module-loader.js"></script>
<script src="system.js"></script>
<script>
  // For AMD/UMD to work
  window.define = System.amdDefine;
  window.require = System.amdRequire;

  // Note that there is no script[src="app-built.js"] tag above
  // since the file seems to be only loadable with System.bundles config
  // for its contents to be resolved correctly

  // bundle name doesn't seem to really matter here
  System.paths['all'] = '/build/static/js/app-built.js';
  System.bundles['all'] = ['backoffice-app/init', 'angular'];

  // Now this works with all the boilerplate stuff above
  System.import('app/app');
  System.import('angular');
</script>

If that is possible, I can imagine how that could speed up the adoption of the SystemJS and entire ES6 System API.

Again, I might have missed something essential so please let me know what are the simplest options for these single-file builds.

guybedford commented 10 years ago

Glad to hear the dependency issue is working, I will merge that soon after some further testing this side.

For the other issue - you can just include the bundle like in your first third code example. Is this not working for you?

It sounds like the issue you are seeing is that you are not including the config file, and it is trying to load Angular from the original files as a separate request (look at the network tab and you will see the requests). As a result it is interpreting it incorrectly as it doesn't have the config to know that it should just use the Angular global only.

VasilioRuzanni commented 10 years ago

@guybedford Thanks a lot for clarification!

But the behavior seems to be different anyway when SystemJS tries to load modules in dev (separate files) and from production build file.

It seems to understand just fine what are exported globals when in dev but for production (module wrapped with System.register) it seems to be using some other resolution algorithm. It successfully finds the angular module in that app-built.js file but executes in some different way so it results in { angular: Object, ng339: 3 } like object. Is that the issue or is expected behavior?

When I do specify the bundles config though, it works just fine. The only thing that bothers me is that necessity to specify that config because I don't provide any additional information about the modules contained inside the app-built.js file that SystemJS wouldn't be able to find out on its own (as I see it) so it feels like workaround. After all, it knows about that module already but just doesn't resolve it correctly (whilst in dev everything is smooth and fine). I'll check the sources of SystemJS on that but any pointers would be welcome.

So the question still remains.

Question 1: Is that Dynamic Workflow 4 is possible in the same form as in that article?

<!doctype html>

<!-- only use Traceur runtime if using other ES6 features -->
<script src="traceur-runtime.js"></script>

<script src="es6-module-loader.js"></script>
<script src="system.js"></script>
<script src="buildfile.js"></script>
<script>
  // Now we can include this bundle after SystemJS, and 
  // have the registry populated correctly
  // Can we?
  System.import('app/app');
</script>

A bit of clarification about UMD-wrapped modules:

// 1. In this example module doesn't return anything, just uses the dependency
// 2. This is the code after 'systemjs-builder'

;(function(root, factory) {
  if (typeof define === 'function' && define.amd) {
    // This is never called in production unless you 
    // explicitly specify 'window.define = System.amdDefine;`
    System.register("app/some-umd-wrapped-module", ['angular'], false, function(__require, __exports, __module) {
      return (function(angular) {
        return factory(angular, document);
      }).call(this, __require('angular'));
    });
  } else if (typeof exports === 'object') {
    factory(require('angular'), document);
  } else {
    // This is called in production but dependencies are undefined
    factory(root.angular, document);
  }
}(this, function(angular, document, undefined) {

  // Module code here...

});

So, the Question 2: Do I really need this code every time any AMD/UMD modules are bundled into app-built.js:

// No require.js was attached at all, so global 'define' and 'require' are just undefined here

window.define = System.amdDefine;
window.require = System.amdRequire;

Or this can be considered a bug as well? Why not expose define and require as globals automatically (just like in dev)?

guybedford commented 10 years ago

In the case where Angular is interpreted incorrectly, did you check the network tab to see if it was being requested separately?

  1. Yes this should work.
  2. I've actually included an AMD wrapper fix in the latest SystemJS builder release (0.1.4) - let me know if this helps. You shouldn't need to define these yourself as they are managed by SystemJS.
VasilioRuzanni commented 10 years ago

Ok, here's the breakdown:

1. Well, it doesn't do it in that "clean way" only with explicit bundles config. Trying to wrap my head around this. The question still stays the "how do I do that?". Just checked:

So, would like to find out where is the difference in globals resolution behavior of SystemJS between raw file (angular.js) and the one wrapped in a System.register.

2. This works pretty fine in general but there is a suggestion, see this PR

VasilioRuzanni commented 10 years ago

Well, @guybedford I've investigated a little more according to point 1 from my previous comment.

These lines of SystemJS definitely decide what will be exported as globals out of module.

Inangular case, while in dev, a singleGlobal is being exported and things are just fine. After systemjs-builder build though, upon execution, ng339 also appears as a property of exports which leads to multipleExports set to true and exports object (with a value of { angular: Object, ng339: 3 }) is returned instead of singleGlobal (which value is angular itself).

I'll dig a bit deeper on what makes that difference between "before" and "after" the build and let you know.

VasilioRuzanni commented 10 years ago

Alright, not sure how exactly module is instantiated when loaded in dev (didn't look at that closely) but it seems that the issue appears because actual "module" code execution happens in different ways when:

So, in case (2) as opposed to case (1), module seems to have already done some of its internal work before actual .retrieveGlobal(...) call. In case of angular module, Angular sets another global ng339 on window directly, which is then recognized by .retrieveGlobal(...) as another "global" hence multipleExports === true.

Not sure exactly how specifying System.bundles['my-bundle'] = ['app', 'angular'] explicitly instead of loading using <script> changes that "execution" flow because when bundles is specified, Angular doesn't seem to execute its code before .retrieveGlobal(...) call thus everything works as expected.

Specifying bundles all the time would work as a workaround but I have a strong feeling that it can be done better in some way since SystemJS already does awesome job recognizing modules anyway.

Any ideas, @guybedford?

guybedford commented 10 years ago

Can you confirm for (1) that you are including the configuration file in production before loading the bundle? You can remove the bundles config entirely, but the build does still need the configuration file to handle normalization and configuration in production.

VasilioRuzanni commented 10 years ago

I can confirm that I'm indeed not adding any kind of configuration into app-built.js file. What kind of configuration should be there? Any pointer would be awesome.

The thing that bothers me is that it actually catches up the correct dependency but executes module registration in different ways somehow.

I believe there might be some kind of "shim" config (maybe it makes sense to provide that to builder itself as a shim property at build step, like we do for require.js? That shim config might have exported globals explicitly specified so that systemjs-builder can "normalize" and "configure" that on its own).

So, basically, the question is what kind of config should be there and what do I need to configure, given that built.js file is loaded already, System.register calls are executed with correct module names, etc. Only runtime globals are resolved by SystemJS seem to be executed differently (when loaded via <script> and via System.import) as I've outlined above.

guybedford commented 10 years ago

Sorry, I thought you were using jspm which was creating the config file for you here, so you would need to load it in production as well.

But it sounds like you have downloaded angular with Bower.

In this case, the way to get this to work is to include angular shim config to ensure it returns the right global. This should be passed with the configuration object into SystemJS builder -

 builder.build('myModule', {
    meta: {
      angular: {
        exports: 'angular'
      }
    }
  }, 'outfile.js')
VasilioRuzanni commented 10 years ago

Yeah, indeed, bower is used as a package manager. I'll try to provide shim config then and let you know if there will be any issue. Thanks a lot!

I think this issue can be closed then. Feel free to close or leave it open if you think some stuff has to be implemented according to our discussion.

guybedford commented 10 years ago

Great, glad to hear that makes sense.

Note that the shim config must be used with the fully normalized name (Promise.resolve(System.normalize('angular')).then(console.log.bind(console)) will give this if you're not sure).

Just let me know if you have any other issues at all.

VasilioRuzanni commented 10 years ago

@guybedford Well, not sure what I have to specify in there but building with systemjs-builder, providing baseURL, paths and meta is pretty much sufficient so everything works without doing any additional magic on paths.

So, I basically add meta: { angular: { exports: 'angular' } } to builder config and everything just works.