-
Notifications
You must be signed in to change notification settings - Fork 28
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
Consider process_common_js_modules #15
Comments
(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:
... 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? |
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). |
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 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 Given that this is still a problem, I think the only benefit of 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. |
@thelgevold I hit an issue with the Without My guess is that this prevents good results when you bring in rxjs: |
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. |
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. |
@kylecordes That is a good point. For now I am adding a simple This is the commonJS version of the hack. If using the ES2015 version of rxjs I just add an ES2015 export instead. |
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. |
@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 |
@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. |
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 |
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. |
Note that tsickle produces externs automatically when you use the |
@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 |
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. |
I'll start another issue(thread) about the general library issue. |
@alexeagle Yeah it looks like there is a 0.9% difference between commonJS and ES2015 (gzipped files) From size_report.sh: ES2015
COMMONJS
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 The throw seems unnecessary, but it compiled down to a standalone throw, so it fails the app unless I comment it out like this:
|
A new closure release finally came out, so I think we should go with your solution Tor. Working on that top-level |
1) don't throw at top-level scope. Closure compiler does not allow this if a goog.module statement is present in the file. We need this modification to use RxJS with angular/tsickle. Addresses angular/tsickle#420 2) refactor the conditional logic for finding the root object See alexeagle/closure-compiler-angular-bundling#15 Closure seems to statically reduce the existing code and eliminates the conditional, making it always throw.
This is looking promising! |
1) don't throw at top-level scope. Closure compiler does not allow this if a goog.module statement is present in the file. We need this modification to use RxJS with angular/tsickle. Addresses angular/tsickle#420 2) refactor the conditional logic for finding the root object See alexeagle/closure-compiler-angular-bundling#15 Closure seems to statically reduce the existing code and eliminates the conditional, making it always throw.
I think we are doing this now, right? Can we close this one? |
Yep. Closing |
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
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
The text was updated successfully, but these errors were encountered: