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

Use Closure Compiler with offline template compiler #8550

Closed
alexeagle opened this Issue May 9, 2016 · 103 comments

Comments

@alexeagle
Collaborator

alexeagle commented May 9, 2016

Angular's new offline compiler was demo'ed at ng-conf day 2 keynote by @robwormald. It generates tree-shakable ES6 code. Four steps are then required:

  • run a tree-shaker to remove ES6 modules not reachable from the entry point, in our demo we showed rollup
  • run a bundler to reduce down the module imports into a declarations in a single file; we showed system.js
  • down-level to ES5 - we showed this with TypeScript
  • minify to shorten symbol names, we used uglify.

This produced a 49k JS file, but requires a lot of configuration.

Google's Closure Compiler (https://developers.google.com/closure/compiler/) produces very small JS. It does all four steps required above, so the configuration should be a lot simpler. We also suspect we can get a smaller binary size for ng2-hello-world, around 36k.

Wiring this up requires:

  • add tsickle's closure annotation helper to `ngc
  • modify Angular's ES6 distribution to be closure-compiler compatible
  • ?? modify ES6 distro of our dependencies (rxjs, zone.js) to be closure-compiler compatible
  • figure out a minimal build (maybe a shell script) that successfully compiles the application together with the framework and its dependencies
  • document how we did it so others can repro
  • bundle externs with Angular to allow Protractor testing (the BrowserNodeGlobal type)
@evmar

This comment has been minimized.

Show comment
Hide comment
@evmar

evmar May 9, 2016

Contributor

The part I think I most need your guidance on is where/how this fits into the larger angular build process.
Do we want to (1) build angular itself as a minified external bundle? or (2) compile angular+user code together into a single bundle? In the latter case we'll need to integrate into gulp or whatever.

Contributor

evmar commented May 9, 2016

The part I think I most need your guidance on is where/how this fits into the larger angular build process.
Do we want to (1) build angular itself as a minified external bundle? or (2) compile angular+user code together into a single bundle? In the latter case we'll need to integrate into gulp or whatever.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 9, 2016

Collaborator

We do distribute Angular bundles, as ES5 with UMD module loader built-in.
However, it would be impossible to pick this back apart again to tree-shake
it, so our plan has been #2.
I'm not sure we should commit to building plugins for the several popular
userland build tools; others in the community will pick that up. I think a
simpler "npm run build && npm run closure-compiler" with shell scripts is
sufficient at this point.

On Mon, May 9, 2016 at 11:26 AM Evan Martin notifications@github.com
wrote:

The part I think I most need your guidance on is where/how this fits into
the larger angular build process.
Do we want to (1) build angular itself as a minified external bundle? or
(2) compile angular+user code together into a single bundle? In the latter
case we'll need to integrate into gulp or whatever.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8550 (comment)

Collaborator

alexeagle commented May 9, 2016

We do distribute Angular bundles, as ES5 with UMD module loader built-in.
However, it would be impossible to pick this back apart again to tree-shake
it, so our plan has been #2.
I'm not sure we should commit to building plugins for the several popular
userland build tools; others in the community will pick that up. I think a
simpler "npm run build && npm run closure-compiler" with shell scripts is
sufficient at this point.

On Mon, May 9, 2016 at 11:26 AM Evan Martin notifications@github.com
wrote:

The part I think I most need your guidance on is where/how this fits into
the larger angular build process.
Do we want to (1) build angular itself as a minified external bundle? or
(2) compile angular+user code together into a single bundle? In the latter
case we'll need to integrate into gulp or whatever.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8550 (comment)

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 9, 2016

Collaborator

FYI @jeffbcross maybe this can make your PWA demo for I/O

Collaborator

alexeagle commented May 9, 2016

FYI @jeffbcross maybe this can make your PWA demo for I/O

@jeffbcross

This comment has been minimized.

Show comment
Hide comment
@jeffbcross

jeffbcross May 9, 2016

Contributor

Yeah that would be awesome if we could have this working by the end of this week.

Contributor

jeffbcross commented May 9, 2016

Yeah that would be awesome if we could have this working by the end of this week.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 12, 2016

Collaborator

Some notes from the hacks I've had to make so far, in the hopes that others can reproduce:

Closure Compiler doesn't handle all ES6 module syntax, and discourages using it. So our strategy so far has been to use goog.module syntax. That's not a --module option to tsc, so we use tsickle to re-write it. That means angular and its dependencies have to be emitted as "Closure ES6".

Need to build closure compiler from HEAD, see https://github.com/google/closure-compiler/blob/master/README.md#building-it-yourself

To build angular into closure ES6, I have local edits in the ngc tool to run tsickle's closure annotation and convertCommonJsToGoogModule. See https://github.com/alexeagle/angular/tree/closure_hack2
Build angular the usual way with ./build.sh and then make the packages installable with for pkg in $(find dist/packages-dist -name package.json); do sed -i .bak 's/\$\$ANGULAR_VERSION\$\$/2.0.0-rc.2-snap/g' $pkg; done

To build rxjs into closure ES6, I have local edits in https://github.com/alexeagle/RxJS/tree/closure - then run npm run build_es6 to get the right files in dist/es6. Then npm run generate_packages to copy in a package.json.

We need a modification to tsickle to workaround ReactiveX/rxjs#1703 - see https://github.com/angular/tsickle/tree/closure

Now I make an example app. Snapshot at https://github.com/alexeagle/closure-compiler-angular-bundling
In the vendor directory I've already installed the locally built closure compiler.jar, angular2 packages, rxjs, and tsickle, with something like
npm install ../angular/dist/packages-dist/{common,compiler_cli,compiler,core,platform-browser,platform-server}
npm install ../rxjs/dist/es6

So you can just npm install and npm run build to reproduce the closure bundle :)

Collaborator

alexeagle commented May 12, 2016

Some notes from the hacks I've had to make so far, in the hopes that others can reproduce:

Closure Compiler doesn't handle all ES6 module syntax, and discourages using it. So our strategy so far has been to use goog.module syntax. That's not a --module option to tsc, so we use tsickle to re-write it. That means angular and its dependencies have to be emitted as "Closure ES6".

Need to build closure compiler from HEAD, see https://github.com/google/closure-compiler/blob/master/README.md#building-it-yourself

To build angular into closure ES6, I have local edits in the ngc tool to run tsickle's closure annotation and convertCommonJsToGoogModule. See https://github.com/alexeagle/angular/tree/closure_hack2
Build angular the usual way with ./build.sh and then make the packages installable with for pkg in $(find dist/packages-dist -name package.json); do sed -i .bak 's/\$\$ANGULAR_VERSION\$\$/2.0.0-rc.2-snap/g' $pkg; done

To build rxjs into closure ES6, I have local edits in https://github.com/alexeagle/RxJS/tree/closure - then run npm run build_es6 to get the right files in dist/es6. Then npm run generate_packages to copy in a package.json.

We need a modification to tsickle to workaround ReactiveX/rxjs#1703 - see https://github.com/angular/tsickle/tree/closure

Now I make an example app. Snapshot at https://github.com/alexeagle/closure-compiler-angular-bundling
In the vendor directory I've already installed the locally built closure compiler.jar, angular2 packages, rxjs, and tsickle, with something like
npm install ../angular/dist/packages-dist/{common,compiler_cli,compiler,core,platform-browser,platform-server}
npm install ../rxjs/dist/es6

So you can just npm install and npm run build to reproduce the closure bundle :)

@dpsthree

This comment has been minimized.

Show comment
Hide comment
@dpsthree

dpsthree May 13, 2016

Contributor

So I would need to have a Java JRE installed to bundle a hello world Angular2 app?

Contributor

dpsthree commented May 13, 2016

So I would need to have a Java JRE installed to bundle a hello world Angular2 app?

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 13, 2016

Collaborator

Closure Compiler is just one option for the
tree-shake/bundle/Es6-to-5/minify pipeline. If you choose the closure
compiler option, you take a JRE dependency for now. But there is also a JS
version in the works:
https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/gwt/client/GwtRunner.java

On Fri, May 13, 2016 at 9:44 AM dpsthree notifications@github.com wrote:

So I would need to have a Java JRE installed to bundle a hello world
Angular2 app?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8550 (comment)

Collaborator

alexeagle commented May 13, 2016

Closure Compiler is just one option for the
tree-shake/bundle/Es6-to-5/minify pipeline. If you choose the closure
compiler option, you take a JRE dependency for now. But there is also a JS
version in the works:
https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/gwt/client/GwtRunner.java

On Fri, May 13, 2016 at 9:44 AM dpsthree notifications@github.com wrote:

So I would need to have a Java JRE installed to bundle a hello world
Angular2 app?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8550 (comment)

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 24, 2016

Collaborator

I wrote some notes on the hacks required to make it work. https://docs.google.com/document/d/17m1GwzXraKgbCkCmH1JnY9IZzPy4cqlpCFVhvlZnOys/edit

Collaborator

alexeagle commented May 24, 2016

I wrote some notes on the hacks required to make it work. https://docs.google.com/document/d/17m1GwzXraKgbCkCmH1JnY9IZzPy4cqlpCFVhvlZnOys/edit

@alexeagle alexeagle modified the milestones: 2.1.0, Angular 2 Final Jul 18, 2016

@jjudd

This comment has been minimized.

Show comment
Hide comment
@jjudd

jjudd Jul 20, 2016

We reproduced @alexeagle's work and with a more up to date version of Angular. The entire build process is automatic, so you can follow everything from source to end result. You can see it in action at https://github.com/lucidsoftware/closure-typescript-example. Instructions for running the example can be found in the README for the example repo. There is a Docker container for the project, so it should be possible for pretty much anyone to try it out.

The above example utilizes Angular 2, clutz, and tsickle. Closure JS is used in Angular 2 TS, which is compiled to Closure compatible JS.

The changes we made to the dependencies to get this to work are largely the same as Alex's. There are a few changes we added and a few changes that are no longer necessary. Regardless, the basic idea is the same. We use a modified ngc or tsickle on Angular 2 and its dependencies to produce Closure compatible versions of those dependencies.

Closure Compiler

Building from head is no longer necessary. Just use one of the versions from June or July.

Angular

https://github.com/lucidsoftware/angular/tree/closure-bundle

The bulk of the work in the Angular 2 source is modifying ngc to produce Closure compilable js by adding a few tsickle steps to tsc-wrapped. There are a few hoops that we jump through in order to use the modified source as input to the TypeScript compiler in one of the tsickle passes. Otherwise, this is pretty straight forward.

We also change certain Angular modules to be compiled to Closure compatible JS by putting an angularCompilerOptions block in the tsconfig.json like so

"angularCompilerOptions": {
  "googleClosureOutput": true
}

Then when you run build.sh those modules have the output you want.

A test utillity file that uses selenium-webdriver is modified because selenium-webdriver is missing and we didn't want to make it closure compatible.

RxJS

https://github.com/lucidsoftware/rxjs/tree/closure-hack

The build process of RxJS is modified to build with the Makefile from our project. About half the build process is in JS and the other half in Make. It's pretty janky.

The only other changes besides the build target are a few small things to make RxJS compile with TypeScript and Closure.

symbol-observable (RxJS dependency)

https://github.com/lucidsoftware/symbol-observable/tree/closure

RxJS now depends on a some code that used to be part of the RxJS source and is now its own module. We modify that module (which is JS) to be compatible with the Closure compiler. There are only two files, so I did this manually.

tsickle

https://github.com/lucidsoftware/tsickle/tree/ignore-type-comments

We modify Tsickle in two ways:

  1. Add a --ignoreTypesInComments option. This prevents tsickle from throwing an error when it encounters jsdoc in comments. This is needed to compile RxJS which has a bunch of jsdoc comments.
  2. Modify the pathToModuleName for the CLI to be the same as the pathToModuleName we use in ngc. That way modules are named the same when run through tsickle or ngc. This should be committed upstream because the current pathToModuleName sometimes produces invalid goog.module names.

jjudd commented Jul 20, 2016

We reproduced @alexeagle's work and with a more up to date version of Angular. The entire build process is automatic, so you can follow everything from source to end result. You can see it in action at https://github.com/lucidsoftware/closure-typescript-example. Instructions for running the example can be found in the README for the example repo. There is a Docker container for the project, so it should be possible for pretty much anyone to try it out.

The above example utilizes Angular 2, clutz, and tsickle. Closure JS is used in Angular 2 TS, which is compiled to Closure compatible JS.

The changes we made to the dependencies to get this to work are largely the same as Alex's. There are a few changes we added and a few changes that are no longer necessary. Regardless, the basic idea is the same. We use a modified ngc or tsickle on Angular 2 and its dependencies to produce Closure compatible versions of those dependencies.

Closure Compiler

Building from head is no longer necessary. Just use one of the versions from June or July.

Angular

https://github.com/lucidsoftware/angular/tree/closure-bundle

The bulk of the work in the Angular 2 source is modifying ngc to produce Closure compilable js by adding a few tsickle steps to tsc-wrapped. There are a few hoops that we jump through in order to use the modified source as input to the TypeScript compiler in one of the tsickle passes. Otherwise, this is pretty straight forward.

We also change certain Angular modules to be compiled to Closure compatible JS by putting an angularCompilerOptions block in the tsconfig.json like so

"angularCompilerOptions": {
  "googleClosureOutput": true
}

Then when you run build.sh those modules have the output you want.

A test utillity file that uses selenium-webdriver is modified because selenium-webdriver is missing and we didn't want to make it closure compatible.

RxJS

https://github.com/lucidsoftware/rxjs/tree/closure-hack

The build process of RxJS is modified to build with the Makefile from our project. About half the build process is in JS and the other half in Make. It's pretty janky.

The only other changes besides the build target are a few small things to make RxJS compile with TypeScript and Closure.

symbol-observable (RxJS dependency)

https://github.com/lucidsoftware/symbol-observable/tree/closure

RxJS now depends on a some code that used to be part of the RxJS source and is now its own module. We modify that module (which is JS) to be compatible with the Closure compiler. There are only two files, so I did this manually.

tsickle

https://github.com/lucidsoftware/tsickle/tree/ignore-type-comments

We modify Tsickle in two ways:

  1. Add a --ignoreTypesInComments option. This prevents tsickle from throwing an error when it encounters jsdoc in comments. This is needed to compile RxJS which has a bunch of jsdoc comments.
  2. Modify the pathToModuleName for the CLI to be the same as the pathToModuleName we use in ngc. That way modules are named the same when run through tsickle or ngc. This should be committed upstream because the current pathToModuleName sometimes produces invalid goog.module names.
@JamesHenry

This comment has been minimized.

Show comment
Hide comment
@JamesHenry

JamesHenry Jul 21, 2016

  • run a tree-shaker to remove ES6 modules not reachable from the entry point, in our demo we showed rollup
  • run a bundler to reduce down the module imports into a declarations in a single file; we showed system.js
  • down-level to ES5 - we showed this with TypeScript
  • minify to shorten symbol names, we used uglify.

Whilst it perhaps cannot match the final output size of the Closure Compiler, webpack 2 (using typescript loader and standard optimize plugins) gets you all of the these steps.

I would argue that the benefit of devs already using (dare I say requiring) a tool like webpack in their stack, and therefore not having to add more (hacked) dependencies, is more important than shaving off extra bytes in the final payload.

Would love to hear your thoughts!

  • run a tree-shaker to remove ES6 modules not reachable from the entry point, in our demo we showed rollup
  • run a bundler to reduce down the module imports into a declarations in a single file; we showed system.js
  • down-level to ES5 - we showed this with TypeScript
  • minify to shorten symbol names, we used uglify.

Whilst it perhaps cannot match the final output size of the Closure Compiler, webpack 2 (using typescript loader and standard optimize plugins) gets you all of the these steps.

I would argue that the benefit of devs already using (dare I say requiring) a tool like webpack in their stack, and therefore not having to add more (hacked) dependencies, is more important than shaving off extra bytes in the final payload.

Would love to hear your thoughts!

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Jul 21, 2016

Collaborator

@jjudd thank you so much for posting your repo and these notes, this is really helpful.

Do you have a minified app size you can share? Perhaps using Brotli compression so we have an apples-to-apples comparison with the hello world closure-ng2 example (which was 26.2k)

@JamesHenry we agree that webpack 2 is the way forward for most developers. Closure compiler is an expert tool and I doubt we can wrap it up in a package that 'just works' for developers who aren't already familiar with debugging the obscure ways its ADVANCED_OPTIMIZATIONS can bust your app.

Collaborator

alexeagle commented Jul 21, 2016

@jjudd thank you so much for posting your repo and these notes, this is really helpful.

Do you have a minified app size you can share? Perhaps using Brotli compression so we have an apples-to-apples comparison with the hello world closure-ng2 example (which was 26.2k)

@JamesHenry we agree that webpack 2 is the way forward for most developers. Closure compiler is an expert tool and I doubt we can wrap it up in a package that 'just works' for developers who aren't already familiar with debugging the obscure ways its ADVANCED_OPTIMIZATIONS can bust your app.

@jjudd

This comment has been minimized.

Show comment
Hide comment
@jjudd

jjudd Jul 21, 2016

@alexeagle We're currently working on getting this to work with ADVANCED_OPTIMIZATIONS. When we get that working I'll go ahead and post some benchmarks.

jjudd commented Jul 21, 2016

@alexeagle We're currently working on getting this to work with ADVANCED_OPTIMIZATIONS. When we get that working I'll go ahead and post some benchmarks.

@evmar

This comment has been minimized.

Show comment
Hide comment
@evmar

evmar Jul 22, 2016

Contributor

Converted the tsickle issue 1 above into a bug report there.

Contributor

evmar commented Jul 22, 2016

Converted the tsickle issue 1 above into a bug report there.

@robwormald

This comment has been minimized.

Show comment
Hide comment
@robwormald

robwormald Jul 23, 2016

Member

closure compiler is now available on npm in pure javascript : https://www.npmjs.com/package/google-closure-compiler-js

Member

robwormald commented Jul 23, 2016

closure compiler is now available on npm in pure javascript : https://www.npmjs.com/package/google-closure-compiler-js

@jjudd

This comment has been minimized.

Show comment
Hide comment
@jjudd

jjudd Aug 18, 2016

Wanted to provide an update:

We have the example repo and the beta Lucidchart editor working with advanced optimizations. We ran a short test using our version with simple optimizations, and our version using advanced optimizations is going out today or tomorrow.

The version of Angular used in the example repo has been upgraded to RC5 (HEAD as of Tuesday Aug 16).

The example repo and its accompanying Docker container have been updated in case anyone wants to try it out. Here's a link to the example repo https://github.com/lucidsoftware/closure-typescript-example

Bundle sizes

The final bundle size for the example project using advanced compilation are listed below. Note: the example repo includes code going from js -> ts using clutz and then ts -> js using tsickle. It is a bit bigger than just a simple hello world app.

Uncompressed: 112K
Brotli quality 10: 29K
Gzip: 34K

Notes on Getting This to Work

Here are the notes on what it took to get advanced optimizations working.In case I forgot something, I'm going to provide the links to all our forks, which have all of our commits.

Example repo

Externs files for Zone, Reflect, and Jasmine were included in the build process, so those function calls were not renamed.

To work around a Closure Compiler bug where static methods get lost, we have a really hacky sed command that we run on the built Angular js files, replacing all lines which include a static keyword with the line preceded by /** @nocollapse */. Here's a link to the Closure bug: google/closure-compiler#1776

Angular

The biggest change we made to Angular is making it use the Tsickle annotated output during compilation. We thought we did this last time, but we had a bug where the annotated output was being thrown away :D

There were a few places that Angular refers to functions as strings. These functions were referred to in other places using dot notation. Accordingly, some references were renamed and others were not. We change Angular to use dot notation everywhere for those functions, so that they are always renamed.

We also updated Angular/Tsickle to not annotate .d.ts files.

We made Angular play nicely with ES6, so it works in uncompiled mode. We change some places where .apply was used on a constructor to use the new keyword.

Tsickle

We fixed a bug in tsickle where the CLI did not always include all the source files during annotation. The list of filenames from the TypeScript Program should have been used, instead we were using a different list of filenames.

Links to all our forks of projects to make this work

Angular: https://github.com/lucidsoftware/angular/tree/closure-bundle
Tsickle: https://github.com/lucidsoftware/tsickle/tree/closure-bundle
RxJS: https://github.com/lucidsoftware/rxjs/tree/closure-hack
symbol-observable: https://github.com/lucidsoftware/symbol-observable/tree/closure
Clutz: https://github.com/lucidsoftware/clutz

jjudd commented Aug 18, 2016

Wanted to provide an update:

We have the example repo and the beta Lucidchart editor working with advanced optimizations. We ran a short test using our version with simple optimizations, and our version using advanced optimizations is going out today or tomorrow.

The version of Angular used in the example repo has been upgraded to RC5 (HEAD as of Tuesday Aug 16).

The example repo and its accompanying Docker container have been updated in case anyone wants to try it out. Here's a link to the example repo https://github.com/lucidsoftware/closure-typescript-example

Bundle sizes

The final bundle size for the example project using advanced compilation are listed below. Note: the example repo includes code going from js -> ts using clutz and then ts -> js using tsickle. It is a bit bigger than just a simple hello world app.

Uncompressed: 112K
Brotli quality 10: 29K
Gzip: 34K

Notes on Getting This to Work

Here are the notes on what it took to get advanced optimizations working.In case I forgot something, I'm going to provide the links to all our forks, which have all of our commits.

Example repo

Externs files for Zone, Reflect, and Jasmine were included in the build process, so those function calls were not renamed.

To work around a Closure Compiler bug where static methods get lost, we have a really hacky sed command that we run on the built Angular js files, replacing all lines which include a static keyword with the line preceded by /** @nocollapse */. Here's a link to the Closure bug: google/closure-compiler#1776

Angular

The biggest change we made to Angular is making it use the Tsickle annotated output during compilation. We thought we did this last time, but we had a bug where the annotated output was being thrown away :D

There were a few places that Angular refers to functions as strings. These functions were referred to in other places using dot notation. Accordingly, some references were renamed and others were not. We change Angular to use dot notation everywhere for those functions, so that they are always renamed.

We also updated Angular/Tsickle to not annotate .d.ts files.

We made Angular play nicely with ES6, so it works in uncompiled mode. We change some places where .apply was used on a constructor to use the new keyword.

Tsickle

We fixed a bug in tsickle where the CLI did not always include all the source files during annotation. The list of filenames from the TypeScript Program should have been used, instead we were using a different list of filenames.

Links to all our forks of projects to make this work

Angular: https://github.com/lucidsoftware/angular/tree/closure-bundle
Tsickle: https://github.com/lucidsoftware/tsickle/tree/closure-bundle
RxJS: https://github.com/lucidsoftware/rxjs/tree/closure-hack
symbol-observable: https://github.com/lucidsoftware/symbol-observable/tree/closure
Clutz: https://github.com/lucidsoftware/clutz

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Sep 9, 2016

Collaborator

So we have a couple examples of this working, plus we use JSCompiler internally at Google so we have many apps doing this (though using the Bazel build system and some not-yet-released Bazel rules).

@robwormald @mprobst should we try to package this up in a more easily reproducible way for external users? I don't think we can claim success and close this issue yet.

Collaborator

alexeagle commented Sep 9, 2016

So we have a couple examples of this working, plus we use JSCompiler internally at Google so we have many apps doing this (though using the Bazel build system and some not-yet-released Bazel rules).

@robwormald @mprobst should we try to package this up in a more easily reproducible way for external users? I don't think we can claim success and close this issue yet.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Sep 20, 2016

Contributor

The JS closure compiler looks really cool. However, I have seen issues where the process runs out of memory when targeting medium sized AOT projects via Rollup. Have you encountered this in your testing?

The Java version does not seem to suffer from this problem though.

google/closure-compiler-js#23

Contributor

thelgevold commented Sep 20, 2016

The JS closure compiler looks really cool. However, I have seen issues where the process runs out of memory when targeting medium sized AOT projects via Rollup. Have you encountered this in your testing?

The Java version does not seem to suffer from this problem though.

google/closure-compiler-js#23

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Sep 20, 2016

Collaborator

I've heard that too. We have had to call node with
--max-old-space-size=4096 to get some tools to work in the past.

On Tue, Sep 20, 2016 at 11:27 AM Torgeir Helgevold notifications@github.com
wrote:

The JS closure compiler looks really cool. However, I have seen issues
where the process runs out of memory with targeting medium sized AOT
projects. Have you encountered this in your testing?

The Java version does not seem to suffer from this problem though.

google/closure-compiler-js#23
google/closure-compiler-js#23


You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I_FDlMdHvHO2YCvypXju9wR8onzuks5qsCWmgaJpZM4IaZIi
.

Collaborator

alexeagle commented Sep 20, 2016

I've heard that too. We have had to call node with
--max-old-space-size=4096 to get some tools to work in the past.

On Tue, Sep 20, 2016 at 11:27 AM Torgeir Helgevold notifications@github.com
wrote:

The JS closure compiler looks really cool. However, I have seen issues
where the process runs out of memory with targeting medium sized AOT
projects. Have you encountered this in your testing?

The Java version does not seem to suffer from this problem though.

google/closure-compiler-js#23
google/closure-compiler-js#23


You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I_FDlMdHvHO2YCvypXju9wR8onzuks5qsCWmgaJpZM4IaZIi
.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Sep 20, 2016

Contributor

I tried max-old-space-size with 8000 and 14000 on a Mac with 16GB of ram, but I still ran out of memory in my sample project.
Java version works though.

Contributor

thelgevold commented Sep 20, 2016

I tried max-old-space-size with 8000 and 14000 on a Mac with 16GB of ram, but I still ran out of memory in my sample project.
Java version works though.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Sep 20, 2016

Collaborator

oh, that seems bad :)
could you file a bug on https://github.com/google/closure-compiler/issues

On Tue, Sep 20, 2016 at 11:52 AM Torgeir Helgevold notifications@github.com
wrote:

I tried max-old-space-size with 8000 and 14000 on a Mac with 16GB of ram,
but I still ran out of memory in my sample project.
Java version works though.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I25uOXJ-Pt8a9WzQQUO1zRx4j1YUks5qsCthgaJpZM4IaZIi
.

Collaborator

alexeagle commented Sep 20, 2016

oh, that seems bad :)
could you file a bug on https://github.com/google/closure-compiler/issues

On Tue, Sep 20, 2016 at 11:52 AM Torgeir Helgevold notifications@github.com
wrote:

I tried max-old-space-size with 8000 and 14000 on a Mac with 16GB of ram,
but I still ran out of memory in my sample project.
Java version works though.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I25uOXJ-Pt8a9WzQQUO1zRx4j1YUks5qsCthgaJpZM4IaZIi
.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Sep 20, 2016

Contributor

Yup I have an active issue there already
google/closure-compiler-js#23

Contributor

thelgevold commented Sep 20, 2016

Yup I have an active issue there already
google/closure-compiler-js#23

@qdouble

This comment has been minimized.

Show comment
Hide comment
@qdouble

qdouble Sep 20, 2016

@thelgevold that may be related to Typescript bug...try building with Typescript nightly build... (unfortunately, ngc doesn't support TS 2.1, so you'd have to go back and forward between that and 2.0.2).

qdouble commented Sep 20, 2016

@thelgevold that may be related to Typescript bug...try building with Typescript nightly build... (unfortunately, ngc doesn't support TS 2.1, so you'd have to go back and forward between that and 2.0.2).

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Sep 20, 2016

Contributor

@qdouble Hm interesting.. Are you saying it might be related to the JavaScript produced by TS 2.0.2?
Because by the time my code enters into rollup/closure compiler land, the code is already in JS format.

Contributor

thelgevold commented Sep 20, 2016

@qdouble Hm interesting.. Are you saying it might be related to the JavaScript produced by TS 2.0.2?
Because by the time my code enters into rollup/closure compiler land, the code is already in JS format.

@qdouble

This comment has been minimized.

Show comment
Hide comment
@qdouble

qdouble Sep 20, 2016

@thelgevold ah, with there is a bug that can stop your code from building if you have a lot of form controls and stuff with 2.0.2...if your files are already JS before you encounter the bug, then it may be something else...but I suppose you could try to use Typescript nightly to turn it into JS and see if it makes any difference.

qdouble commented Sep 20, 2016

@thelgevold ah, with there is a bug that can stop your code from building if you have a lot of form controls and stuff with 2.0.2...if your files are already JS before you encounter the bug, then it may be something else...but I suppose you could try to use Typescript nightly to turn it into JS and see if it makes any difference.

@IgorMinar IgorMinar removed the P2: required label Oct 4, 2016

@u12206050

This comment has been minimized.

Show comment
Hide comment
@u12206050

u12206050 Oct 7, 2016

Hi Guys, all of this sounds very interesting and we are looking into this to improve the performance of our website. What is the current status of the custom angular builds you are making. Will you be making a build for Angular v 2.0.0 ?

Hi Guys, all of this sounds very interesting and we are looking into this to improve the performance of our website. What is the current status of the custom angular builds you are making. Will you be making a build for Angular v 2.0.0 ?

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Oct 7, 2016

Collaborator

I will try to have some update on this for ngEurope...

On Fri, Oct 7, 2016, 1:48 AM Gerard A Lamusse notifications@github.com
wrote:

Hi Guys, all of this sounds very interesting and we are looking into this
to improve the performance of our website. What is the current status of
the custom angular builds you are making. Will you be making a build for
Angular v 2.0.0 ?


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I1TZZEGiuBu8zJe2BtYEzilr-VPHks5qxgdTgaJpZM4IaZIi
.

Collaborator

alexeagle commented Oct 7, 2016

I will try to have some update on this for ngEurope...

On Fri, Oct 7, 2016, 1:48 AM Gerard A Lamusse notifications@github.com
wrote:

Hi Guys, all of this sounds very interesting and we are looking into this
to improve the performance of our website. What is the current status of
the custom angular builds you are making. Will you be making a build for
Angular v 2.0.0 ?


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#8550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC5I1TZZEGiuBu8zJe2BtYEzilr-VPHks5qxgdTgaJpZM4IaZIi
.

@jjudd

This comment has been minimized.

Show comment
Hide comment
@jjudd

jjudd Oct 7, 2016

Hi @u12206050, I took a crack getting this upgraded to Angular 2.0.1 a couple weekends ago. I got it about 2/3 of the way there. The example works with simple optimizations, but not with advanced optimizations.

I'm no longer actively working on this, but another team here at Lucid I spoke with today wants to take a crack at it soon. If I get another free weekend, I'd like to finish moving it to 2.0.1. So whoever gets to it first.

In the meantime, if you would like a bit more info on this, you can check out the blog post we wrote on it at https://www.lucidchart.com/techblog/2016/09/26/improving-angular-2-load-times/. I hope that helps.

If you run into any issues and want to bounce some questions off of me, please feel free. It is likely going to be a bit of an adventure hooking it up to your build system, though. Best of luck! :)

jjudd commented Oct 7, 2016

Hi @u12206050, I took a crack getting this upgraded to Angular 2.0.1 a couple weekends ago. I got it about 2/3 of the way there. The example works with simple optimizations, but not with advanced optimizations.

I'm no longer actively working on this, but another team here at Lucid I spoke with today wants to take a crack at it soon. If I get another free weekend, I'd like to finish moving it to 2.0.1. So whoever gets to it first.

In the meantime, if you would like a bit more info on this, you can check out the blog post we wrote on it at https://www.lucidchart.com/techblog/2016/09/26/improving-angular-2-load-times/. I hope that helps.

If you run into any issues and want to bounce some questions off of me, please feel free. It is likely going to be a bit of an adventure hooking it up to your build system, though. Best of luck! :)

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Feb 6, 2017

Contributor

@alexeagle Would it be more optimized to somehow transpile to the square bracket equivalent for these cases?

Not sure if it will work, but my understanding is that externs should be limited to external framework references like global variables "owned" by other frameworks.

I think Closure can do a better job at optimizing quoted props through a combination of a single full length reference, and shortened internal references to the same property.

I believe externs are less optimized since it won't touch any references to the property name. Meaning 50 refs to firstName will remain as firstName, while square brackets might leave a single public ref as firstName and mangle the other 49 internal refs.

Contributor

thelgevold commented Feb 6, 2017

@alexeagle Would it be more optimized to somehow transpile to the square bracket equivalent for these cases?

Not sure if it will work, but my understanding is that externs should be limited to external framework references like global variables "owned" by other frameworks.

I think Closure can do a better job at optimizing quoted props through a combination of a single full length reference, and shortened internal references to the same property.

I believe externs are less optimized since it won't touch any references to the property name. Meaning 50 refs to firstName will remain as firstName, while square brackets might leave a single public ref as firstName and mangle the other 49 internal refs.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Feb 6, 2017

Collaborator

@thelgevold that's true, the externs approach marks that property name as un-renamable everywhere.
We don't have any way to automatically re-write dot-access to quoted-access, though I suppose with the type system we could see the declare interface and know when to do it.

But after compression, that may not matter? If firstName goes into the dictionary at the header of the compressed file, it may not make much difference how many times it's referenced in the code.

FWIW this is the approach we take right now internally at Google. But I think we should experiment with your suggestion if we get time.

Collaborator

alexeagle commented Feb 6, 2017

@thelgevold that's true, the externs approach marks that property name as un-renamable everywhere.
We don't have any way to automatically re-write dot-access to quoted-access, though I suppose with the type system we could see the declare interface and know when to do it.

But after compression, that may not matter? If firstName goes into the dictionary at the header of the compressed file, it may not make much difference how many times it's referenced in the code.

FWIW this is the approach we take right now internally at Google. But I think we should experiment with your suggestion if we get time.

@b-strauss

This comment has been minimized.

Show comment
Hide comment
@b-strauss

b-strauss Feb 6, 2017

I'm pretty sure closure does not optimize quoted props in any way. Closure just removes the square bracket notation from this: model['firstname'] to this x.firstname.

AFAIK the "clean" way with extern files and the square notation way does produce the same result.

I'm pretty sure closure does not optimize quoted props in any way. Closure just removes the square bracket notation from this: model['firstname'] to this x.firstname.

AFAIK the "clean" way with extern files and the square notation way does produce the same result.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Feb 6, 2017

Collaborator

the problem is that notModel.firstName will also be y.firstname instead of y.z

Collaborator

alexeagle commented Feb 6, 2017

the problem is that notModel.firstName will also be y.firstname instead of y.z

@b-strauss

This comment has been minimized.

Show comment
Hide comment
@b-strauss

b-strauss Feb 6, 2017

Closure should be able to nominally distinguish the types of these instances by the types provided in the externs. That's how it worked in the old "global" world. I have no idea how to generate externs for classes that have the same name but come from different modules.

b-strauss commented Feb 6, 2017

Closure should be able to nominally distinguish the types of these instances by the types provided in the externs. That's how it worked in the old "global" world. I have no idea how to generate externs for classes that have the same name but come from different modules.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Feb 6, 2017

Contributor

@alexeagle You are probably right. There might not be much of a gain after gzip.

It would probably be complicated as well since you would most likely have to discover all the "same" firstName references and rewrite them to ['firstName'] all the way through. Otherwise there would be a mix of .firstName notation and ['firstName'], which would break at runtime.

@b-strauss Closure will convert ['firstName'] to .firstName. However, here is the optimization theory behind [''] vs externs: https://developers.google.com/closure/compiler/docs/api-tutorial3
I have seen this behavior in some of my experiments, but it may not be worth the effort here.

Contributor

thelgevold commented Feb 6, 2017

@alexeagle You are probably right. There might not be much of a gain after gzip.

It would probably be complicated as well since you would most likely have to discover all the "same" firstName references and rewrite them to ['firstName'] all the way through. Otherwise there would be a mix of .firstName notation and ['firstName'], which would break at runtime.

@b-strauss Closure will convert ['firstName'] to .firstName. However, here is the optimization theory behind [''] vs externs: https://developers.google.com/closure/compiler/docs/api-tutorial3
I have seen this behavior in some of my experiments, but it may not be worth the effort here.

@lypanov

This comment has been minimized.

Show comment
Hide comment
@lypanov

lypanov Feb 6, 2017

For mobile larger JS source file will still take longer to parse. It's not just a question of post gzip size.

lypanov commented Feb 6, 2017

For mobile larger JS source file will still take longer to parse. It's not just a question of post gzip size.

@b-strauss

This comment has been minimized.

Show comment
Hide comment
@b-strauss

b-strauss Feb 6, 2017

@thelgevold I know the theory behind it. :)

I just wrote a little test:

This app code:

/**
 * @constructor
 */
function NotMyModel() {
  this.firstname = 'foo';
}

console.log(new NotMyModel().firstname);
console.log(new MyModel().firstname);

together with this third-party code:

<script>
  function MyModel() {
    this.firstname = 'foo';
  }
</script>

and these externs:

/**
 * @constructor
 */
function MyModel() {
}

/**
 * @type {string|null}
 */
MyModel.prototype.firstname;

produces the following output:

(function () {
  console.log("foo");
  console.log((new MyModel).firstname);
})();

Closure is able to distinguish the two firstnames based on the nominal types, and even inlines the first one. As I said, in the old "global" world this was not a problem, because best practice was to always prefix your types com.bstrauss.MyClass.

The question is how should tsickle generate externs in a world where types from two different modules could have the same name?

AFAIK there is no equivalent goog.module for externs.

@thelgevold I know the theory behind it. :)

I just wrote a little test:

This app code:

/**
 * @constructor
 */
function NotMyModel() {
  this.firstname = 'foo';
}

console.log(new NotMyModel().firstname);
console.log(new MyModel().firstname);

together with this third-party code:

<script>
  function MyModel() {
    this.firstname = 'foo';
  }
</script>

and these externs:

/**
 * @constructor
 */
function MyModel() {
}

/**
 * @type {string|null}
 */
MyModel.prototype.firstname;

produces the following output:

(function () {
  console.log("foo");
  console.log((new MyModel).firstname);
})();

Closure is able to distinguish the two firstnames based on the nominal types, and even inlines the first one. As I said, in the old "global" world this was not a problem, because best practice was to always prefix your types com.bstrauss.MyClass.

The question is how should tsickle generate externs in a world where types from two different modules could have the same name?

AFAIK there is no equivalent goog.module for externs.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Feb 6, 2017

Contributor

@b-strauss Thanks, for sharing that experiment.

My original comment was more about how [''] might optimize multiple references to the same MyModel.firstName property. Not so much about optimizing for crossover wrt same names across different model objects.

My understanding is that if you have 10 MyModel().firstname refs in your code, using [''] could potentially allow Closure to create an internal short version alias. That way the net size is smaller since it preserves firstName where other code expects it, but benefits from the shorter name for private/local refs. It's my understanding that an extern will not give you that benefit since it leaves all references alone.

Contributor

thelgevold commented Feb 6, 2017

@b-strauss Thanks, for sharing that experiment.

My original comment was more about how [''] might optimize multiple references to the same MyModel.firstName property. Not so much about optimizing for crossover wrt same names across different model objects.

My understanding is that if you have 10 MyModel().firstname refs in your code, using [''] could potentially allow Closure to create an internal short version alias. That way the net size is smaller since it preserves firstName where other code expects it, but benefits from the shorter name for private/local refs. It's my understanding that an extern will not give you that benefit since it leaves all references alone.

@mprobst

This comment has been minimized.

Show comment
Hide comment
@mprobst

mprobst Feb 7, 2017

Contributor

@b-strauss my plan was to tackle this in angular/tsickle#352, by mangling the externs name and then aliasing locally. That is, for a file foo/bar/baz.ts:

/** @externs */
function tsickle_from_foo_bar_baz_ts_MyModel() {}
/** @type {string} */
tsickle_from_foo_bar_baz_ts_MyModel.prototype.firstName;

And at the usage site:

goog.module('foo.bar.baz');

// Alias:
var MyModel = tsickle_from_foo_bar_baz_ts_MyModel;

// ... later on ...
new MyModel().firstName;

Let's take the discussion to that issue, this thread is becoming a bit unfocused.

Contributor

mprobst commented Feb 7, 2017

@b-strauss my plan was to tackle this in angular/tsickle#352, by mangling the externs name and then aliasing locally. That is, for a file foo/bar/baz.ts:

/** @externs */
function tsickle_from_foo_bar_baz_ts_MyModel() {}
/** @type {string} */
tsickle_from_foo_bar_baz_ts_MyModel.prototype.firstName;

And at the usage site:

goog.module('foo.bar.baz');

// Alias:
var MyModel = tsickle_from_foo_bar_baz_ts_MyModel;

// ... later on ...
new MyModel().firstName;

Let's take the discussion to that issue, this thread is becoming a bit unfocused.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 5, 2017

A while back in this thread there is a mention of discussion about how well accepted a Closure Compiler based build chain might be by the community. Even if it remains secondary numerically to the Webpack stack in Angular-CLI, I suspect it will still get decent adoption if it provides a smaller output. Size still matters a lot, particularly on mobile.

@alexeagle re "third party libraries will have to stay outside the closure compilation" - That's probably a good starting point, but if things get cleaned up where Closure can be used almost trivially on Angular application code, I believe this will create considerable pressure on Angular add-on library makers (though perhaps not the broader ecosystem) to verify their stuff works with Closure. The same thing has been happening over the last few months regarding AOT, in the early fall most add-on libraries said "AOT?" but today most of them that I stumble across have things working with AOT or are well aware and heading that direction.

If anyone been following this work in the various repos linked in this issue over the last year, I think the latest ones I've seen above are still pointing at the "-builds" packages, which were necessary until recently. But now Angular 4 rc ships with ES2015 modules, so that is no longer necessary. Here's a branch I've updated to work with the latest, in the process of catching up on understanding what's been going on with Closure use.

https://github.com/kylecordes/closure-compiler-angular-bundling/tree/update-versions

It feels like we are on the verge of a production-grade Angular build chain consisting of just ngc (with tsc inside) plus Closure. That is a nice, short stack.

A while back in this thread there is a mention of discussion about how well accepted a Closure Compiler based build chain might be by the community. Even if it remains secondary numerically to the Webpack stack in Angular-CLI, I suspect it will still get decent adoption if it provides a smaller output. Size still matters a lot, particularly on mobile.

@alexeagle re "third party libraries will have to stay outside the closure compilation" - That's probably a good starting point, but if things get cleaned up where Closure can be used almost trivially on Angular application code, I believe this will create considerable pressure on Angular add-on library makers (though perhaps not the broader ecosystem) to verify their stuff works with Closure. The same thing has been happening over the last few months regarding AOT, in the early fall most add-on libraries said "AOT?" but today most of them that I stumble across have things working with AOT or are well aware and heading that direction.

If anyone been following this work in the various repos linked in this issue over the last year, I think the latest ones I've seen above are still pointing at the "-builds" packages, which were necessary until recently. But now Angular 4 rc ships with ES2015 modules, so that is no longer necessary. Here's a branch I've updated to work with the latest, in the process of catching up on understanding what's been going on with Closure use.

https://github.com/kylecordes/closure-compiler-angular-bundling/tree/update-versions

It feels like we are on the verge of a production-grade Angular build chain consisting of just ngc (with tsc inside) plus Closure. That is a nice, short stack.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 9, 2017

I published the follow repository showing how to use 4rc.2, ngc (AOT), Closure Compiler:

https://github.com/OasisDigital/angular-aot-closure

Technologically this has little to offer over Alex's, but it has a different goal. I'm trying to make this repository contain plenty of comments and text explaining how and why things are. Therefore please feel free to open an issue on it, if there is anything not sufficiently explained in the comments or README.

I published the follow repository showing how to use 4rc.2, ngc (AOT), Closure Compiler:

https://github.com/OasisDigital/angular-aot-closure

Technologically this has little to offer over Alex's, but it has a different goal. I'm trying to make this repository contain plenty of comments and text explaining how and why things are. Therefore please feel free to open an issue on it, if there is anything not sufficiently explained in the comments or README.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 16, 2017

@nosachamos Wild guess: something in the toolchain (probably tsickle or Closure?) did not anticipate a parameter named this. My suggestion is:

  • Edit your local copy of RxJS source, change that parameter name, confirm fixed. If so then...
  • Put in an issue (or even a PR) to RxJS to rename this this and any similar this parameters. Hopefully the RxJS team will see the merit in such a change;possibly a good word from the Angular team ( @alexeagle ?) on such an issue will help.
  • Put in an issue with a simple repro case to Closure, about the parameter named this... and it doesn't repro there, look elsewhere in the tool chain (tsickle?).

kylecordes commented Mar 16, 2017

@nosachamos Wild guess: something in the toolchain (probably tsickle or Closure?) did not anticipate a parameter named this. My suggestion is:

  • Edit your local copy of RxJS source, change that parameter name, confirm fixed. If so then...
  • Put in an issue (or even a PR) to RxJS to rename this this and any similar this parameters. Hopefully the RxJS team will see the merit in such a change;possibly a good word from the Angular team ( @alexeagle ?) on such an issue will help.
  • Put in an issue with a simple repro case to Closure, about the parameter named this... and it doesn't repro there, look elsewhere in the tool chain (tsickle?).
@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@kylecordes Ops, I found my issue and deleted the comment I guess as you were typing.

For the record, my issue was that an argument named "this" was being removed during transpilation.

It turns out, closure was invoking that function using call, so this was actually set that way and needed not be passed in. If I rename "this" to "this2", the argument is kept. I guess the transpiler is smarter than I thought.

My issue was within ngrx effects, not rxjs. Ngrx/effects accesses the effects through their names, using the brackets notation, which is broken by the closure compiler when it renames all the things. To prevent the issue, in the constructor of your classes containing your effects, assign their names to "this" using brackets notation:

export class MyEffects {

  @Effect()
  doSomething$: Observable<Action> = this.actions$ ...

  constructor() {
    this['doSomething$'] = this.doSomething$;
  }

}

So I'm almost up and running on a real world app using the ngrx stack. I'm writing what I believe to be the last manual extern required for my case. If this all runs well, I'll create a branch on the sample app here with the ngrx stack as a demo. I'm using my own fork of ngrx/core and ngrx/store while my PRs for expanding wildcard exports are not merged, but other than that, I think this will go well.

I'll keep you guys posted.

nosachamos commented Mar 16, 2017

@kylecordes Ops, I found my issue and deleted the comment I guess as you were typing.

For the record, my issue was that an argument named "this" was being removed during transpilation.

It turns out, closure was invoking that function using call, so this was actually set that way and needed not be passed in. If I rename "this" to "this2", the argument is kept. I guess the transpiler is smarter than I thought.

My issue was within ngrx effects, not rxjs. Ngrx/effects accesses the effects through their names, using the brackets notation, which is broken by the closure compiler when it renames all the things. To prevent the issue, in the constructor of your classes containing your effects, assign their names to "this" using brackets notation:

export class MyEffects {

  @Effect()
  doSomething$: Observable<Action> = this.actions$ ...

  constructor() {
    this['doSomething$'] = this.doSomething$;
  }

}

So I'm almost up and running on a real world app using the ngrx stack. I'm writing what I believe to be the last manual extern required for my case. If this all runs well, I'll create a branch on the sample app here with the ngrx stack as a demo. I'm using my own fork of ngrx/core and ngrx/store while my PRs for expanding wildcard exports are not merged, but other than that, I think this will go well.

I'll keep you guys posted.

@evmar

This comment has been minimized.

Show comment
Hide comment
@evmar

evmar Mar 16, 2017

Contributor

Can you file a bug against tsickle with more information on the this issue? Our intent is that you shouldn't need to manually rename variables like this.

Is it possible to fix ngrx to not use brackets?

Contributor

evmar commented Mar 16, 2017

Can you file a bug against tsickle with more information on the this issue? Our intent is that you shouldn't need to manually rename variables like this.

Is it possible to fix ngrx to not use brackets?

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 16, 2017

@nosachamos based on your deleted comment, and my maybe-should-be deleted comment, is there any issue to report? (per @evmar )

@nosachamos based on your deleted comment, and my maybe-should-be deleted comment, is there any issue to report? (per @evmar )

@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@kylecordes Maybe,
but what I think it's happening is that the transpiler can figure out the function has "this" set by the call invocation. So it can in this case safely remove the argument, as this will be whatever is passed to call. That would not be true if the argument gets renamed, so it keeps the argument. I think in "this" particular case, the transpiler is fine. In fact, renaming the argument doesn't fix the problem, as the real issue was the bracket access.

@evmar I believe so. This bracket access is quite central to their operation but I suspect annotating the underlying dictionary with closure's @dict annotation would suffice to fix the issue. I will give this a try and submit a PR to them if that works. Else, I'll submit a PR to their README with a note on the bracket workaround when using ngrx/effects with closure.

@kylecordes Maybe,
but what I think it's happening is that the transpiler can figure out the function has "this" set by the call invocation. So it can in this case safely remove the argument, as this will be whatever is passed to call. That would not be true if the argument gets renamed, so it keeps the argument. I think in "this" particular case, the transpiler is fine. In fact, renaming the argument doesn't fix the problem, as the real issue was the bracket access.

@evmar I believe so. This bracket access is quite central to their operation but I suspect annotating the underlying dictionary with closure's @dict annotation would suffice to fix the issue. I will give this a try and submit a PR to them if that works. Else, I'll submit a PR to their README with a note on the bracket workaround when using ngrx/effects with closure.

@evmar

This comment has been minimized.

Show comment
Hide comment
@evmar

evmar Mar 16, 2017

Contributor

Note that any Closure type annotation added to TS code is stripped by Tsickle, so adding an @dict there won't help.

Contributor

evmar commented Mar 16, 2017

Note that any Closure type annotation added to TS code is stripped by Tsickle, so adding an @dict there won't help.

@awerlang

This comment has been minimized.

Show comment
Hide comment
@awerlang

awerlang Mar 16, 2017

Contributor

My issue was within ngrx effects, not rxjs. Ngrx/effects accesses the effects through their names, using the brackets notation, which is broken by the closure compiler when it renames all the things. To prevent the issue, in the constructor of your classes containing your effects, assign their names to "this" using brackets notation:

@nosachamos Won't the same issue happen with @Input() and @Output()? Another question would be: should closure rename decorated fields?

Contributor

awerlang commented Mar 16, 2017

My issue was within ngrx effects, not rxjs. Ngrx/effects accesses the effects through their names, using the brackets notation, which is broken by the closure compiler when it renames all the things. To prevent the issue, in the constructor of your classes containing your effects, assign their names to "this" using brackets notation:

@nosachamos Won't the same issue happen with @Input() and @Output()? Another question would be: should closure rename decorated fields?

@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@evmar I'm transpiling with removeComments: false and all my closure annotations are passed onto the ES6 code I feed closure, so that should be fine. I'm using @dict in other places, and it's fine.

@awerlang no, the @dict is a jsdoc style annotation, not an actual annotation like @input. These closure annotations go on comments.

nosachamos commented Mar 16, 2017

@evmar I'm transpiling with removeComments: false and all my closure annotations are passed onto the ES6 code I feed closure, so that should be fine. I'm using @dict in other places, and it's fine.

@awerlang no, the @dict is a jsdoc style annotation, not an actual annotation like @input. These closure annotations go on comments.

@evmar

This comment has been minimized.

Show comment
Hide comment
@evmar

evmar Mar 16, 2017

Contributor

@nosachamos our test case is this input https://github.com/angular/tsickle/blob/master/test_files/jsdoc/jsdoc.ts producing this output
https://github.com/angular/tsickle/blob/master/test_files/jsdoc/jsdoc.js
where you'll see we munge most annotations. Maybe there's a bug though.

Contributor

evmar commented Mar 16, 2017

@nosachamos our test case is this input https://github.com/angular/tsickle/blob/master/test_files/jsdoc/jsdoc.ts producing this output
https://github.com/angular/tsickle/blob/master/test_files/jsdoc/jsdoc.js
where you'll see we munge most annotations. Maybe there's a bug though.

@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@evmar oh, I'm compiling with ngc and using

    "target": "es6",
    "module": "es2015",

and

  "angularCompilerOptions": {
    "skipMetadataEmit": true,
    "annotationsAs": "static fields",
    "annotateForClosureCompiler": true
  },

So I get just ES6, no goog.modules.. so everything seems to be working as intended, and I get the closure annotations. This is for RxJs. For packages that interact with angular (components, modules, etc), then skipMetadataEmit is set to false.

nosachamos commented Mar 16, 2017

@evmar oh, I'm compiling with ngc and using

    "target": "es6",
    "module": "es2015",

and

  "angularCompilerOptions": {
    "skipMetadataEmit": true,
    "annotationsAs": "static fields",
    "annotateForClosureCompiler": true
  },

So I get just ES6, no goog.modules.. so everything seems to be working as intended, and I get the closure annotations. This is for RxJs. For packages that interact with angular (components, modules, etc), then skipMetadataEmit is set to false.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 16, 2017

@nosachamos as I understand, the tsickle step can't do it's thing, and won't do its thing when run via ngc, unless you are using "module": "common". Tsickle enforces this limitation when run by its own CLI, does it gain the ability to work with es2015 module output when run via ngc? Do you see tsickle-generated JSDoc in the es2015 module output?

@nosachamos as I understand, the tsickle step can't do it's thing, and won't do its thing when run via ngc, unless you are using "module": "common". Tsickle enforces this limitation when run by its own CLI, does it gain the ability to work with es2015 module output when run via ngc? Do you see tsickle-generated JSDoc in the es2015 module output?

@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@kylecordes yeah, I get the same exact output you get in the demo repo you posted above compiling with tsc-wrapped and the rxjs-tsickle tsconfig.

@kylecordes yeah, I get the same exact output you get in the demo repo you posted above compiling with tsc-wrapped and the rxjs-tsickle tsconfig.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle Mar 16, 2017

Collaborator

could we move bugs to another issue? it doesn't scale to address individual problems on the tracking issue. Thanks!

Collaborator

alexeagle commented Mar 16, 2017

could we move bugs to another issue? it doesn't scale to address individual problems on the tracking issue. Thanks!

@nosachamos

This comment has been minimized.

Show comment
Hide comment
@nosachamos

nosachamos Mar 16, 2017

@alexeagle absolutely. I posted here initially because I thought this was relevant (a problem in how rxjs was declaring arguments named "this" that were being lost). There's nothing to follow up, but if something comes up we'll do in a separate issue.

@alexeagle absolutely. I posted here initially because I thought this was relevant (a problem in how rxjs was declaring arguments named "this" that were being lost). There's nothing to follow up, but if something comes up we'll do in a separate issue.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 17, 2017

@nosachamos Indeed... I just found that indeed, tsickle-via-ngc or -cli-wrapped can handle module:es2015.

@alexeagle Ah, oops. At 90+ comments, I didn't realize it was a tracking issue. Will make new issues if they come up.

@nosachamos Indeed... I just found that indeed, tsickle-via-ngc or -cli-wrapped can handle module:es2015.

@alexeagle Ah, oops. At 90+ comments, I didn't realize it was a tracking issue. Will make new issues if they come up.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Mar 22, 2017

Contributor

Took another look at the generated bundle. Noticed that there is definitely room for further code reductions here.

It's not always easy for Closure to identify unnecessary code since the existing code structure sometimes makes it seem like the code is needed.

Based on the POC app in @alexeagle 's repo I picked a few unnecessarily included elements and made a few temporary tweaks to the source to force them out of the bundle.

Seems like there are a few repeating patterns here:

A service class may not be needed by the application, but if the service is added to a provider array along the way, it may cause the class to make it into the bundle. Examples of this are AnimationDriver and AnimationQueue. By removing the provider references to these classes I am able to get these classes out of the bundle.

The sample app in Alex's repo does not do any QueryList querying, but a lot of the Query related code is still in the bundle.
In this case Closure can't exclude the code because the code lives in a switch statement where multiple scenarios are handled.
Take a look at createViewNodes in core.js and the NodeType.Query clause in particular.

case NodeType.Query:
       nodeData = (createQuery());
       break;

The conditions in this method are resolved at runtime , so Closure has to include all the code to satisfy all the conditions in the switch. I removed this particular clause and the code was excluded from the bundle.

These are just two simple examples, but if you continue down this path, it will eventfully add up to noticeable reductions in size. I didn't go very far with this sample, but I probably removed roughly 1k by addressing 4-5 cases like this.

I also did a second experiment where I removed a bunch of code manually (without using Closure). I got my app down to 16.6k and I believe there is still room for removing more. Here is the sample: http://www.syntaxsuccess.com/viewarticle/minimal-angular-application . This particular experiment is a major hack, but at least it proves that you can stand up an Angular app with very little code.

To help with some of these challenges I was thinking it would perhaps make sense to extend AoT to generate more of the framework code - not just the view code.

Just thinking out loud here, but couldn't the ngc compiler in theory generate code for stuff like createViewNodes based on the needs of specific applications. That way if your app doesn't need Pipes, QueryList or whatever, those particular case statements would be omitted from the generated code.

Thoughts?

Contributor

thelgevold commented Mar 22, 2017

Took another look at the generated bundle. Noticed that there is definitely room for further code reductions here.

It's not always easy for Closure to identify unnecessary code since the existing code structure sometimes makes it seem like the code is needed.

Based on the POC app in @alexeagle 's repo I picked a few unnecessarily included elements and made a few temporary tweaks to the source to force them out of the bundle.

Seems like there are a few repeating patterns here:

A service class may not be needed by the application, but if the service is added to a provider array along the way, it may cause the class to make it into the bundle. Examples of this are AnimationDriver and AnimationQueue. By removing the provider references to these classes I am able to get these classes out of the bundle.

The sample app in Alex's repo does not do any QueryList querying, but a lot of the Query related code is still in the bundle.
In this case Closure can't exclude the code because the code lives in a switch statement where multiple scenarios are handled.
Take a look at createViewNodes in core.js and the NodeType.Query clause in particular.

case NodeType.Query:
       nodeData = (createQuery());
       break;

The conditions in this method are resolved at runtime , so Closure has to include all the code to satisfy all the conditions in the switch. I removed this particular clause and the code was excluded from the bundle.

These are just two simple examples, but if you continue down this path, it will eventfully add up to noticeable reductions in size. I didn't go very far with this sample, but I probably removed roughly 1k by addressing 4-5 cases like this.

I also did a second experiment where I removed a bunch of code manually (without using Closure). I got my app down to 16.6k and I believe there is still room for removing more. Here is the sample: http://www.syntaxsuccess.com/viewarticle/minimal-angular-application . This particular experiment is a major hack, but at least it proves that you can stand up an Angular app with very little code.

To help with some of these challenges I was thinking it would perhaps make sense to extend AoT to generate more of the framework code - not just the view code.

Just thinking out loud here, but couldn't the ngc compiler in theory generate code for stuff like createViewNodes based on the needs of specific applications. That way if your app doesn't need Pipes, QueryList or whatever, those particular case statements would be omitted from the generated code.

Thoughts?

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Mar 26, 2017

Contributor

Based on what I learned from manually removing code in the bundles, I have applied similar modifications to the bundles in the closure fork.

I put the tweaks in a branch in case you are interested in seeing the results: https://github.com/thelgevold/closure-compiler-angular-bundling-old/tree/tweaking-src

Here is a diff of the original bundles vs the tweaked bundles: thelgevold/closure-compiler-angular-bundling-old@d1b3954

If you want to try it, just replace the default bundles with the modified ones. I am assuming Angular 4.0.0 for this experiment.

Here are the numbers with the new bundles:

The new size is now ~15k gzipped, and slightly smaller with brotli.

++ ls -alH dist/bundle.js dist/bundle.js.brotli dist/bundle.js.gz dist/bundle.js.map
-rw-r--r--  1 tor  staff   47487 Mar 26 13:58 dist/bundle.js
-rw-r--r--  1 tor  staff   13979 Mar 26 13:58 dist/bundle.js.brotli
-rw-r--r--  1 tor  staff   15482 Mar 26 13:58 dist/bundle.js.gz
-rw-r--r--  1 tor  staff  133888 Mar 26 13:58 dist/bundle.js.map
++ for script in dist/bundle.js node_modules/zone.js/dist/zone.min.js
++ gzip --keep -f node_modules/zone.js/dist/zone.min.js
++ bro --force --quality 10 --input node_modules/zone.js/dist/zone.min.js --output node_modules/zone.js/dist/zone.min.js.brotli
++ ls -alH node_modules/zone.js/dist/zone.min.js node_modules/zone.js/dist/zone.min.js.brotli node_modules/zone.js/dist/zone.min.js.gz
-rw-r--r--  1 tor  staff  29634 Mar 25 11:00 node_modules/zone.js/dist/zone.min.js
-rw-------  1 tor  staff   8759 Mar 26 13:58 node_modules/zone.js/dist/zone.min.js.brotli
-rw-r--r--  1 tor  staff   9516 Mar 25 11:00 node_modules/zone.js/dist/zone.min.js.gz

Obviously it's hard to replicate this without restructuring the Angular source, but it's at least interesting to see how small a basic app can get.

Not sure how useful this is, but it's at least a fun experiment :-)

In summary:
A lot of the savings are from removing code from switch/if-else structures. Closure can't currently infer that some of the options are not possible at runtime for a given application. Another case is provider arrays populated with unused services.

Contributor

thelgevold commented Mar 26, 2017

Based on what I learned from manually removing code in the bundles, I have applied similar modifications to the bundles in the closure fork.

I put the tweaks in a branch in case you are interested in seeing the results: https://github.com/thelgevold/closure-compiler-angular-bundling-old/tree/tweaking-src

Here is a diff of the original bundles vs the tweaked bundles: thelgevold/closure-compiler-angular-bundling-old@d1b3954

If you want to try it, just replace the default bundles with the modified ones. I am assuming Angular 4.0.0 for this experiment.

Here are the numbers with the new bundles:

The new size is now ~15k gzipped, and slightly smaller with brotli.

++ ls -alH dist/bundle.js dist/bundle.js.brotli dist/bundle.js.gz dist/bundle.js.map
-rw-r--r--  1 tor  staff   47487 Mar 26 13:58 dist/bundle.js
-rw-r--r--  1 tor  staff   13979 Mar 26 13:58 dist/bundle.js.brotli
-rw-r--r--  1 tor  staff   15482 Mar 26 13:58 dist/bundle.js.gz
-rw-r--r--  1 tor  staff  133888 Mar 26 13:58 dist/bundle.js.map
++ for script in dist/bundle.js node_modules/zone.js/dist/zone.min.js
++ gzip --keep -f node_modules/zone.js/dist/zone.min.js
++ bro --force --quality 10 --input node_modules/zone.js/dist/zone.min.js --output node_modules/zone.js/dist/zone.min.js.brotli
++ ls -alH node_modules/zone.js/dist/zone.min.js node_modules/zone.js/dist/zone.min.js.brotli node_modules/zone.js/dist/zone.min.js.gz
-rw-r--r--  1 tor  staff  29634 Mar 25 11:00 node_modules/zone.js/dist/zone.min.js
-rw-------  1 tor  staff   8759 Mar 26 13:58 node_modules/zone.js/dist/zone.min.js.brotli
-rw-r--r--  1 tor  staff   9516 Mar 25 11:00 node_modules/zone.js/dist/zone.min.js.gz

Obviously it's hard to replicate this without restructuring the Angular source, but it's at least interesting to see how small a basic app can get.

Not sure how useful this is, but it's at least a fun experiment :-)

In summary:
A lot of the savings are from removing code from switch/if-else structures. Closure can't currently infer that some of the options are not possible at runtime for a given application. Another case is provider arrays populated with unused services.

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Mar 27, 2017

As I understand from @thelgevold 's work, there is considerable output size reduction, with relatively minor-to-moderate adjustment to Angular:

  • Accommodate fewer features with if and switch in the source code.
  • Pre-configured access again to fewer of those features, in the provider arrays built in.
  • Adjust the compiler (affecting both JIT and AOT) to generate the additional bits of code to wire up those features/providers in the generated code... if the features are actually used.

I'm interested in what someone on the core team thinks of this, whether it really is minor-to-moderate and whether there is interest in going down this path.

As I understand from @thelgevold 's work, there is considerable output size reduction, with relatively minor-to-moderate adjustment to Angular:

  • Accommodate fewer features with if and switch in the source code.
  • Pre-configured access again to fewer of those features, in the provider arrays built in.
  • Adjust the compiler (affecting both JIT and AOT) to generate the additional bits of code to wire up those features/providers in the generated code... if the features are actually used.

I'm interested in what someone on the core team thinks of this, whether it really is minor-to-moderate and whether there is interest in going down this path.

@alexeagle

This comment has been minimized.

Show comment
Hide comment
@alexeagle

alexeagle May 2, 2017

Collaborator

Today we got a release of rxjs that works with closure compiler, and I've updated the example repo
https://github.com/alexeagle/closure-compiler-angular-bundling

The externs are cleaned up as well - Angular and Zone.js both distribute the needed externs in their respective packages.
We'll add some documentation and an announcement about the support shortly.

Collaborator

alexeagle commented May 2, 2017

Today we got a release of rxjs that works with closure compiler, and I've updated the example repo
https://github.com/alexeagle/closure-compiler-angular-bundling

The externs are cleaned up as well - Angular and Zone.js both distribute the needed externs in their respective packages.
We'll add some documentation and an announcement about the support shortly.

@buu700

This comment has been minimized.

Show comment
Hide comment
@buu700

buu700 May 6, 2017

Now that all of this is out of the way, at this point do you have an idea of how it will end up integrating with the rest of the existing tooling?

e.g. might there eventually be an option added to @ngtools/webpack to automatically run the AOT output through Closure (and/or some documented configuration steps for webpack-closure-compiler)? Or is this something that will necessarily have to supersede webpack?

buu700 commented May 6, 2017

Now that all of this is out of the way, at this point do you have an idea of how it will end up integrating with the rest of the existing tooling?

e.g. might there eventually be an option added to @ngtools/webpack to automatically run the AOT output through Closure (and/or some documented configuration steps for webpack-closure-compiler)? Or is this something that will necessarily have to supersede webpack?

@vforv

This comment has been minimized.

Show comment
Hide comment
@vforv

vforv May 21, 2017

Is possible to implement ngx-bootstrap with Closure?

https://github.com/valor-software/ngx-bootstrap

vforv commented May 21, 2017

Is possible to implement ngx-bootstrap with Closure?

https://github.com/valor-software/ngx-bootstrap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment