Consider process_common_js_modules

[thelgevold]

I did an experiment today using process_common_js_modules to integrate the default commonjs build of rxjs with the sample app.

There is a bug in the current build of Closure compiler that prevents this from working. However the bug seems to be fixed in Head, so I built a new version of Closure from source.

Not sure what you guys think about going in this direction vs waiting for an ES2015 build?

What I like about process_common_js_modules is that it may at least be a solution for future commonJS libs that we may encounter. It also solves the issue of having to add an extraneous export statement to the rxjs modules to make it work with ES2015.

There is no impact to imports in user code. We can still use regular ES2015 import statements since Closure seems to do the conversion.

There is one issue that I encountered in a rxjs module called root.js

"use strict";
/**
 * window: browser in DOM main thread
 * self: browser in WebWorker
 * global: Node.js/other
 */
exports.root = (typeof window == 'object' && window.window === window && window
    || typeof self == 'object' && self.self === self && self
    || typeof global == 'object' && global.global === global && global);
if (!exports.root) {
    // Comment out this thow and the app works
    throw new Error('RxJS could not find any global context (window, self, global)');
}
//# sourceMappingURL=root.js.map

The following code is for some reason compiled down to a standalone throw. If I comment out the throw in the rxjs file it works though.

I guess this code is not Closure safe. Not sure why it needs this throw. Wouldn't one of these variables always be defined?
(e.g. root = window || global || self).

I don't see the need for this throw, but I might be missing something as I only looked at it briefly.

Anyway. I have a working sample in my fork if you are interested: https://github.com/thelgevold/closure-compiler-angular-bundling-old/tree/rxjs-commonjs

Since it's a bit of a pain to do Closure releases, I have included the new Closure jar file in my branch.

Here is the commit diff: thelgevold@15a52a7

[kylecordes]

(There is an issue requesting more frequent, or per-commit Closure builds - there is no technological obstacle to it, just waiting for someone to do it. I think it has to come from the Closure team, not a contributor, because it involved admin access the Closure publishing mechanism, NPM account, etc.)

This is slick! Here is a question/caveat this approach, though. I had imagined the goal of Closure builds, to have:

• Angular
• Application code
• Typescript-based libraries commonly used with Angular

... all (eventually) made Closure-advanced-compatible, all processed with ngc/tsickle, to enable maximal Closure optimization across a wide swath of the code in a typical application.

The rest of the ecosystem, the thousands of less-used libraries, would get less optimization, of course. It should all be usable, with the help of occasional Closure extern files.

How much of the benefit of such an approach do we give up, if we punt on everything but Angular itself, or everything but Angular and app code?

[thelgevold]

Hm.. I don't think bringing in commonJS impacts Closure optimizations. It doesn't depend on ES2015 like other optimizers do. From what I can tell RxJs seems compatible with Closure (expect for the root module I described above).

[thelgevold]

Bummer. I spoke too soon about getting rid of the extraneous rxjs export from imported modules.

This is still an issue with both commonjs and es2015.

The issue is with imports from rxjs/add/operator and rxjs/add/observable.

These operators/observables don't export anything, but have side effects since the prototype on the global Observable object is mutated in the module.

People use these imports to get the chaining effect in the rxjs api.

Not handling modules with no export seems like a bug in Closure?

Alternatively people can import from rxjs/operator and rxjs/observable, but this doesn't give you the chaining of operators.

Given that this is still a problem, I think the only benefit of process_common_js_modules now, is that we don't need a separate build of rxjs. Not sure if that is enough to warrant the change if there is an rxjs-es in the works?

In any event. I am curious if Closure will fix the issues with modules with no exports, or if we have to rely on patching the rxjs modules with an export?

The latter is definitely undesirable, unless it can be added to the rxjs distribution.

[kylecordes]

@thelgevold I hit an issue with the rxjs/add/operator problem, totally apart from --process_common_js_modules. Closure --js_module_root is broken for imports that operate only for side-effects - and I'd guess this is still the case even with --process_common_js_modules.

Without --process_common_js_modules, the fix is to rearrange files for that rxjs is sitting there there at the directory at which you run Closure - not under vendor or anywhere else. You can see the gymnastics I did for that here:

https://github.com/OasisDigital/angular-aot-closure/blob/master/tsconfig.rxjs-es6-closure.json

My guess is that this prevents good results when you bring in rxjs:

https://github.com/thelgevold/closure-compiler-angular-bundling-old/blob/rxjs-commonjs/closure.conf#L27

[kylecordes]

Also in general I have hit many more issues, trying to get Closure up and running on a slightly non-trivial app than a trivial one.

[kylecordes]

commonJS impacts Closure optimizations

Regarding this bit - yes, certainly Closure can work on vanilla JS code. But for its best results with Advanced Optimizations, it should consume JS code annotated with its JSDoc-derived comments which encode a type system. The more information it has about the code (types), the more it can discard as it compiles. Hence the existence of the type docs, tsickle, etc. Vanilla CommonJS code already published "out there" is very unlikely to have these things that enable optimal Closure results.

[thelgevold]

@kylecordes That is a good point.
In this example it looks like the results are similar (at least according to manifest, renaming reports), but it would be interesting to measure the impact of the annotations in general.

For now I am adding a simple exports.__CLOSURE_WORKAROUND__ = true; to all my imported rxjs operators. This is a hack, but it works for now.

This is the commonJS version of the hack. If using the ES2015 version of rxjs I just add an ES2015 export instead.

[kylecordes]

I wonder if RxJS could be persuaded to include such a thing... or whether Closure might fix the bug soon. I hope soon the answer is not a custom local build or local edit of of RxJS to work with Angular + Closure.

[thelgevold]

@kylecordes Out of curiosity, what were the challenges you encountered in your non-trivial app when converting?

I spent some time today converting a fairly comprehensive demo application and for the most part things went smooth.

It's not an enormous app, but it is a fairly comprehensive app as it uses http, forms, reactive forms, http, routing etc.

Aside from the rxjs trickery I had to "square bracket" protect things like external api responses. This is fine for now, but I believe someone brought up a convention where we would handle this with declared interfaces. Let the compiler define externs for these objects instead. Couldn't quite get that to work, but this is an issue for another day.

Anyway, the results were quite good. The size of the app went down from 160k/151k (Webpack/Rollup) to 107k with Closure.

I have included links to all three build versions here: http://www.syntaxsuccess.com/viewarticle/closure-compiler-vs-rollup-vs-webpack

[kylecordes]

@thelgevold Here's a quick partial answer. I was using advanced optimizations, as you were. Yes, the square bracket thing hit all over. It took some experimentation even on a small app. Code that works with CLI (and in this repo demonstrated with rollup, but irrelevant):

https://github.com/OasisDigital/angular-aot-es2015-rollup/tree/master/app

Same code modified to work with Closure:

https://github.com/OasisDigital/angular-aot-closure/tree/master/app

Here are a couple of images showing specific diffs - unfortunately the two repos are not related in a way that lets me make a good hub link to show the diff.

The important thing was that it is not obvious where you need to switch to square brackets; it is not obvious whether the Angular compiler is doing the "right thing" yet, in terms of how it generates code to refer to fields that you refer to in a template, whether it refers to them in a square bracket kind of way or a field reference kind of way. It might be correct, but even being reasonably experience with these things, I had to tinker quite a bit to get working results.

I also tried setting up interfaces, hoping that if I declare the types robustly, they would get translated to Closure by tsickle and "just work"; I couldn't get that to work either.

The example diffs shown here are reasonably representative of that tinkering.

It sounds like I got similar results to you. I interpreted them as: this is going to be tricky to teach developers how to follow the "rules", how to understand when something does not work with Closure, what and where they might need to adjust to make it start working.

I'm hoping that as the core Angular team focus now includes Closure ("ABC" at ng-conf), perhaps certain improvements in the compiler and in tsickle will make the whole thing "just work" if data is typed with TypeScript types all the way through. I believe that should be the case, but I think there are some rough edges somewhere making it not work yet.

[thelgevold]

Yep, I encountered similar scenarios.

Ideally the compiler will do this via interfaces. For now I guess we can do this either via square bracketing, or perhaps by making our own externs.

I think the case can be made that externs are maybe more appropriate than square brackets when
referring to external things like api responses.

[alexeagle]

Hey there, you are both awesome for pushing this forward!

I tried to keep track of the pending issues at the bottom of my README.md - you'll find the issue for the rxjs modules with no export, for example.

Re. making it easier to get the property naming syntax right - yes! I'd like to do some static analysis of this. We haven't found the right formula yet - I posted microsoft/TypeScript#14267 as one suggestion. Maybe tsickle can just fix this, in cases where we have good type information.

[alexeagle]

Note that tsickle produces externs automatically when you use the declare keyword.
In TypeScript, declare interface F {} and interface F {} are identical, because interface has no value-side. So we introduced a semantic difference for tsickle's benefit.
However, this is not hooked up in ngc right now :(

[kylecordes]

@alexeagle It's tedious to work on this stuff at the moment - because every example / example-repo has to reproduce a pile of workarounds to various issues. I'm really hoping a bunch of the straightforward ones (issues in other projects you linked from your TODO) get resolved from the other end in the coming weeks. That will make it much easier to focus on the more essential issues.

Property naming - see #16

[alexeagle]

back to the original topic (to keep the issue threads organized)

@thelgevold do you have a bundle size comparison of using RxJS default commonjs/es5 distro? I think it's about 1% bigger which we could probably live with. If I had to choose between ironing out this kink, and waiting for RxJS to publish something different, I'd probably do your fix.

[kylecordes]

I'll start another issue(thread) about the general library issue.

[thelgevold]

@alexeagle Yeah it looks like there is a 0.9% difference between commonJS and ES2015 (gzipped files)

From size_report.sh:

ES2015

-rw-------  1 tor  staff  29348 Apr 10 21:31 dist/bundle.js.brotli
-rw-r--r--  1 tor  staff  33094 Apr 10 21:31 dist/bundle.js.gz

COMMONJS

-rw-------  1 tor  staff  29718 Apr 10 21:28 dist/bundle.js.brotli
-rw-r--r--  1 tor  staff  33395 Apr 10 21:28 dist/bundle.js.gz

As it stands now: The commonJs version requires a custom build of Closure (added to my fork). There was also one issue where I had to comment out a throw in rxjs/util/root.js.

The throw seems unnecessary, but it compiled down to a standalone throw, so it fails the app unless I comment it out like this:

exports.root = (typeof window == 'object' && window.window === window && window
    || typeof self == 'object' && self.self === self && self
    || typeof global == 'object' && global.global === global && global);
if (!exports.root) {
    //throw new Error('RxJS could not find any global context (window, self, global)');
}

[alexeagle]

A new closure release finally came out, so I think we should go with your solution Tor. Working on that top-level throw statement in ReactiveX/rxjs#2546 (I can't figure out under what conditions it strips the conditional either...)

[thelgevold]

This is looking promising!

[alexeagle]

I think we are doing this now, right? Can we close this one?

[thelgevold]

Yep. Closing