feat(closure): fix #727, add zone_externs.js for closure compiler #731

JiaLiPassion · 2017-04-11T17:25:52Z

add closure compiler externs file in dist/zone_externs.js
add test script in travis.
now we have 2 cases.

normal case, can compile and keep the names defined in extern
fail case, will throw error if the extern file is broken (definition error).

alexeagle · 2017-04-12T00:40:35Z

package.json

@@ -43,7 +43,9 @@
  "bugs": {
    "url": "https://github.com/angular/zone.js/issues"
  },
-  "dependencies": {},
+  "dependencies": {


should be a devDependency right?

yes, you are right, I will change it.

and you can remove the dependencies{} block too

alexeagle · 2017-04-12T00:44:08Z

lib/zone.ts

@@ -136,11 +136,11 @@
 interface Zone {
  /**
   *
-   * @returns {Zone} The parent Zone.
+   * @returns {?Zone} The parent Zone.


what is the reasoning for updating all the inline types?

If the externs file is independent of the .ts file,

we'll have to maintain two different copies of the same info, and

nothing enforces the JSDoc annotations in the .ts file are correct, so we will probably fail to maintain them

angular/tsickle can generate externs from the TS types, so we could just remove the JSDoc annotations if we were using the extern generator.

I know you already did a lot of work here, sorry to have to ask this!

Thank for the review, I am new to closure compiler, so I just check the document and add the type I can find.

So I just revert all those inline type changes from zone.ts, is that correct?

alexeagle · 2017-04-12T00:46:18Z

scripts/closure/closure_compiler.sh

+  # Flags to simplify debugging.
+  "--formatting=PRETTY_PRINT"
+
+  "--externs=test/closure/zone_externs_failed.js"


nit: I'd have one options block with the options that are in common (including the zone.closure.js input file)
then when you call compiler.jar below, give the options that differ between the two cases

@alexeagle , yes, you are right, I will make it change that way.

alexeagle · 2017-04-12T00:46:59Z

scripts/closure/closure_compiler.sh

+# compile closure test source file
+tsc -p .
+# Run the Google Closure compiler java runnable.
+java -jar node_modules/google-closure-compiler/compiler.jar --flagfile $closureFlags 'build/test/closure/zone.closure.js'


nit: I prefer to have the flagfile be an actual file in this directory, then you don't have to use the fancy bash array construct that few people understand

yes, I just copied from material, I also prefer use flagfile directly, I will modify it.

alexeagle · 2017-04-12T00:49:05Z

test/closure/zone_externs_failed.js

+*/
+
+/**
+ * @fileoverview Externs for zone.js


this file also looks like a lot of maintenance work. What is different between this and the other one? Could we get the same test coverage by just calling closure with no externs at all?

Yeah, you are right , I will change it that way.

JiaLiPassion · 2017-04-12T01:55:38Z

@alexeagle , thank you the review, I will make the changes of the comments you write above, and about the dist/zone_externs.js, is that ok? It can keep the name unchanged, I am not sure the @type @interface @record definitions are correct or not, please help me to check it , thank you.

Updated:
@alexeagle , I just commit a new version and have changed files based on your comment, please review. thank you.

alexeagle

looking good, thanks! I'll let Misko look as well...

alexeagle · 2017-04-12T16:24:59Z

dist/zone_externs.js

+*/
+
+/**
+ * @fileoverview Externs for zone.js


we can verify that these work in a real project, like overwrite https://github.com/alexeagle/closure-compiler-angular-bundling/blob/master/vendor/zone_externs.js with yours and re-build

sure, I will check it too

alexeagle · 2017-04-12T16:25:34Z

package.json

@@ -43,7 +43,9 @@
  "bugs": {
    "url": "https://github.com/angular/zone.js/issues"
  },
-  "dependencies": {},
+  "dependencies": {


and you can remove the dependencies{} block too

alexeagle · 2017-04-12T16:26:02Z

package.json

@@ -52,6 +53,7 @@
    "concurrently": "^2.2.0",
    "conventional-changelog": "^1.1.0",
    "es6-promise": "^3.0.2",
+    "google-closure-compiler": "^20170218.0.0",


a new April release just came out, may as well upgrade this now

Ok, got it. and the earlier version has a empty

dependencies: {}

I just reverted it to that way.

alexeagle · 2017-04-12T16:27:44Z

scripts/closure/closure_compiler.sh

+  exit 1
+fi
+
+# Run the Google Closure compiler java runnable with error zone externs.


update comment - "without externs"

I just updated the comment.

JiaLiPassion · 2017-04-12T17:55:19Z

@alexeagle, I have modified the comment and update package.json, please review.

alexeagle · 2017-04-12T18:35:11Z

scripts/closure/closure_compiler.sh

+
+if [ $? -eq 1 ]
+then
+  echo "Successfully detect closure compiler error with wrong zone externs"


here too, wrong -> missing

alexeagle · 2017-04-12T18:35:16Z

scripts/closure/closure_compiler.sh

+then
+  echo "Successfully detect closure compiler error with wrong zone externs"
+else
+  echo "failed to detect closure compiler error with wrong zone externs" 


@alexeagle , thanks, I will modify it now.

googlebot added the cla: yes label Apr 11, 2017

JiaLiPassion mentioned this pull request Apr 11, 2017

Distribute closure externs for Zone #727

Closed

alexeagle reviewed Apr 12, 2017

View reviewed changes

JiaLiPassion force-pushed the closure branch 2 times, most recently from 138f5c4 to ed36db1 Compare April 12, 2017 02:36

alexeagle reviewed Apr 12, 2017

View reviewed changes

JiaLiPassion force-pushed the closure branch from ed36db1 to b7d0669 Compare April 12, 2017 17:22

alexeagle approved these changes Apr 12, 2017

View reviewed changes

JiaLiPassion force-pushed the closure branch 2 times, most recently from fc3f529 to 1423dfe Compare April 12, 2017 18:45

feat(closure): fix angular#727, add zone_externs.js for closure compiler

fe622fe

JiaLiPassion force-pushed the closure branch from 1423dfe to fe622fe Compare April 12, 2017 19:00

mhevery merged commit b60e9e6 into angular:master Apr 17, 2017

JiaLiPassion deleted the closure branch May 6, 2017 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(closure): fix #727, add zone_externs.js for closure compiler #731

feat(closure): fix #727, add zone_externs.js for closure compiler #731

JiaLiPassion commented Apr 11, 2017 •

edited

Loading

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

alexeagle Apr 12, 2017

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017 •

edited

Loading

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

JiaLiPassion commented Apr 12, 2017 •

edited

Loading

alexeagle left a comment

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

alexeagle Apr 12, 2017

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017 •

edited

Loading

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017 •

edited

Loading

JiaLiPassion commented Apr 12, 2017

alexeagle Apr 12, 2017

alexeagle Apr 12, 2017

JiaLiPassion Apr 12, 2017

feat(closure): fix #727, add zone_externs.js for closure compiler #731

feat(closure): fix #727, add zone_externs.js for closure compiler #731

Conversation

JiaLiPassion commented Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion commented Apr 12, 2017 • edited Loading

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

JiaLiPassion commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion commented Apr 11, 2017 •

edited

Loading

JiaLiPassion Apr 12, 2017 •

edited

Loading

JiaLiPassion commented Apr 12, 2017 •

edited

Loading

JiaLiPassion Apr 12, 2017 •

edited

Loading

JiaLiPassion Apr 12, 2017 •

edited

Loading