Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: run closure-compiler #3789

Merged
merged 2 commits into from
Mar 29, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ env:
# Order: a slower build first, so that we don't occupy an idle travis worker waiting for others to complete.
- MODE=lint
- MODE=aot
- MODE=closure-compiler
- MODE=payload
- MODE=e2e
- MODE=saucelabs_required
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,12 @@
"conventional-changelog": "^1.1.0",
"dgeni": "^0.4.7",
"dgeni-packages": "^0.16.5",
"firebase": "^3.7.2",
"firebase-admin": "^4.1.2",
"firebase-tools": "^2.2.1",
"firebase": "^3.7.2",
"fs-extra": "^2.0.0",
"glob": "^7.1.1",
"google-closure-compiler": "^20170218.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you use npm shrinkwrap to avoid surprises when the version moves?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are only using a yarn.lock. But yeah might be a good idea to introduce such a file. @jelbourn for confirmation?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't experienced issues with this such that I have an opinion one way or the other. Is it reasonable to leave it until we run into a problem?

"google-cloud": "^0.48.0",
"gulp": "^3.9.1",
"gulp-clean": "^0.3.2",
Expand Down
2 changes: 2 additions & 0 deletions scripts/ci/build-and-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ elif is_aot; then
$(npm bin)/gulp ci:aot
elif is_payload; then
$(npm bin)/gulp ci:payload
elif is_closure_compiler; then
./scripts/closure-compiler/build-devapp-bundle.sh
else
$(npm bin)/gulp ci:test
fi
Expand Down
4 changes: 4 additions & 0 deletions scripts/ci/sources/mode.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ is_aot() {
[[ "$MODE" = aot ]]
}

is_closure_compiler() {
[[ "$MODE" = closure-compiler ]]
}

is_payload() {
[[ "$MODE" = payload ]]
}
91 changes: 91 additions & 0 deletions scripts/closure-compiler/build-devapp-bundle.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
#!/bin/bash

# Script that bundles the dev-app using the Google Closure compiler.
# This is script is used to verify closure-compatiblity of all Material components.
Copy link
Contributor

Choose a reason for hiding this comment

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

it only does the first bits of closure verification of course, we don't know if the property renaming did the right thing. can we also run some of the integration tests on the result (or does this PR do that?)

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR just bundles our demo-app that uses Angular Material. So not really sure if this is enough for the variable renaming. Can be a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to run Closure Compiler over the e2e-app and run the e2e tests over that in order to ensure that everything is working; I think this PR is a good first step to ensure that that the code at least passes.


set -e -o pipefail

# Go to the project root directory
cd $(dirname $0)/../..

# Build the demo-app and also create the release output.
$(npm bin)/gulp build:devapp
$(npm bin)/gulp :package:release

# Rebuild demo-app with ES2015 modules. Closure compiler is then able to parse imports.
$(npm bin)/tsc -p src/demo-app/ --target ES2015 --module ES2015

# Re-compile RxJS sources into ES2015. Otherwise closure compiler can't parse it properly.
$(npm bin)/ngc -p scripts/closure-compiler/tsconfig-rxjs.json

# Create a list of all RxJS source files.
rxjsSourceFiles=$(find dist/packages/rxjs -name '*.js');

# Due a Closure Compiler issue https://github.com/google/closure-compiler/issues/2247
# we need to add exports to the different RxJS ES2015 files.
for i in $rxjsSourceFiles; do
echo "export var __CLOSURE_WORKAROUND__" >> $i
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I guess I didn't encode the workaround into angular or my example app because we didn't happen to load one of the rx operators?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we are using a few and those couldn't be imported. Took me a while to find that issue :)

done

OPTS=(
Copy link
Contributor

Choose a reason for hiding this comment

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

now that we use a flagfile maybe it would be better to formulate this with

cat > flagfile << EOF
...
EOF

instead of a bash array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure. I just tried it and it didn't look very pretty inside of the Bash script. I might be doing something wrong? What advantage does it have?

"--language_in=ES6_STRICT"
"--language_out=ES5"
"--compilation_level=ADVANCED_OPTIMIZATIONS"
"--js_output_file=dist/closure/closure-bundle.js"
"--variable_renaming_report=dist/closure/variable_renaming_report"
"--property_renaming_report=dist/closure/property_renaming_report"
"--warning_level=QUIET"
"--rewrite_polyfills=false"

# List of path prefixes to be removed from ES6 & CommonJS modules.
"--js_module_root=dist/packages"
"--js_module_root=dist/release"
"--js_module_root=node_modules/@angular/core"
"--js_module_root=node_modules/@angular/common"
"--js_module_root=node_modules/@angular/compiler"
"--js_module_root=node_modules/@angular/forms"
"--js_module_root=node_modules/@angular/http"
"--js_module_root=node_modules/@angular/router"
"--js_module_root=node_modules/@angular/platform-browser"
"--js_module_root=node_modules/@angular/platform-browser/animations"
"--js_module_root=node_modules/@angular/platform-browser-dynamic"
"--js_module_root=node_modules/@angular/animations"
"--js_module_root=node_modules/@angular/animations/browser"

# Flags to simplify debugging.
"--formatting=PRETTY_PRINT"
"--debug"

# Include the Material FESM bundle
dist/release/@angular/material.js

# Include all Angular FESM bundles.
node_modules/@angular/core/@angular/core.js
node_modules/@angular/common/@angular/common.js
node_modules/@angular/compiler/@angular/compiler.js
node_modules/@angular/forms/@angular/forms.js
node_modules/@angular/http/@angular/http.js
node_modules/@angular/router/@angular/router.js
node_modules/@angular/platform-browser/@angular/platform-browser.js
node_modules/@angular/platform-browser/@angular/platform-browser/animations.js
node_modules/@angular/platform-browser-dynamic/@angular/platform-browser-dynamic.js
node_modules/@angular/animations/@angular/animations.js
node_modules/@angular/animations/@angular/animations/browser.js

# Include other dependencies like Zone.js and RxJS
node_modules/zone.js/dist/zone.js
$rxjsSourceFiles

# Include all files from the demo-app package.
$(find dist/packages/demo-app -name '*.js')

"--entry_point=./dist/packages/demo-app/main.js"
"--dependency_mode=STRICT"
)

# Write closure flags to a closure flagfile.
closureFlags=$(mktemp)
echo ${OPTS[*]} > $closureFlags

# Run the Google Closure compiler java runnable.
java -jar node_modules/google-closure-compiler/compiler.jar --flagfile $closureFlags
16 changes: 16 additions & 0 deletions scripts/closure-compiler/tsconfig-rxjs.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe extend from the tsconfig.json in rxjs sources? Shouldn't you share most of their options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah would be nice, but unfortunately RxJS doesn't ship their tsconfig.json. We only have the plain sources from the NPM package. See here

"compilerOptions": {
"module": "es2015",
"outDir": "../../dist/packages/rxjs",
"target": "es2015",
Copy link
Contributor

Choose a reason for hiding this comment

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

actually the target should be es5 - that's what they are likely to ship

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

"lib": ["es2015", "dom"]
},
"files": [
"../../node_modules/rxjs/src/Rx.ts"
],
"angularCompilerOptions": {
"annotateForClosureCompiler": true,
"skipMetadataEmit": true,
"skipTemplateCodegen": true
}
}