Update Closure Tools dependencies and add arguments to closure_js_binary() #910

Dominator008 · 2016-02-16T06:26:16Z

Update Closure Compiler to v20160208
Update Closure Library to v20160208
Update Closure Stylesheets to 1.1.0

Add "--language_in" and "--language_out" to closure_js_binary.bzl
With these flags users can transpile their closure_js_binary() from
ES6 to ES3.

Change "--manage_closure_dependencies" to "--dependency_mode=LOOSE"
because the former has been deprecated in Closure Compiler v20160208.

Update Closure Compiler to v20160208 Update Closure Library to v20160208 Update Closure Stylesheets to 1.1.0

damienmg · 2016-02-16T09:23:46Z

Looks good for me, I would like @kamalmarhubi to have a look too

damienmg · 2016-02-17T13:58:39Z

Ooops I ping the wrong person, that @kjiwa that I wanted to have a look at this PR

kjiwa · 2016-02-17T19:41:56Z

tools/build_rules/closure/closure_js_binary.bzl

@@ -46,6 +48,21 @@ _COMPILATION_LEVELS = {
  "advanced": ["--compilation_level=ADVANCED"]
 }

+_LANGUAGE_LEVELS = {


Nit: These aren't "levels". How about naming it _LANGUAGES _SUPPORTED_LANGUAGES?

Done. I guess you meant _SUPPORTED_LANGUAGES?

I meant to put an "or" between the two suggestions. :-/

Kamil: if you can merge it, would be awesome :)

On Wed, Feb 17, 2016 at 10:13 PM Michael Zhou notifications@github.com
wrote:

In tools/build_rules/closure/closure_js_binary.bzl
#910 (comment):

@@ -46,6 +48,21 @@ _COMPILATION_LEVELS = {
"advanced": ["--compilation_level=ADVANCED"]
}

+_LANGUAGE_LEVELS = {

I see :)

—
Reply to this email directly or view it on GitHub
https://github.com/bazelbuild/bazel/pull/910/files#r53232161.

kjiwa · 2016-02-17T19:43:33Z

Looks great. Thank you for the change. I just had a minor nitpick about the naming of your constant. If you don't mind updating that I'll be glad to LGTM your pull request.

With these flags users can transpile their closure_js_binary() from ES6 to ES3. Change "--manage_closure_dependencies" to "--dependency_mode=LOOSE" because the former has been deprecated in Closure Compiler v20160208.

kjiwa · 2016-02-17T21:10:00Z

Thanks for the update. LGTM.

@damienmg Do you want to merge the request or shall I?

Update Closure Tools dependencies and add language arguments to closure_js_binary()

damienmg · 2016-02-17T21:24:10Z

Wait no!

damienmg · 2016-02-17T21:24:16Z

gosh

This reverts commit 3f70577, reversing changes made to cbbbf2e. Accidental merge, merged pull request have to go through googlesource first

damienmg · 2016-02-17T21:36:20Z

Sorry I had to revert the change and it will be imported back thank for your contribution!

Dominator008 · 2016-02-17T21:39:19Z

@damienmg No worries. For future contributions, do you prefer GitHub PRs or your Gerrit instance at https://bazel-review.googlesource.com?

jart · 2016-02-17T22:16:29Z

tools/build_rules/closure/closure_repositories.bzl

-      sha256 = "e4e0cb49ad21ec26dd47693bdbd48f67aefe2d94fe8d9239312d2bcc74986538",
-      url = "http://dl.google.com/closure-compiler/compiler-20150729.zip",
+      sha256 = "215ba5df026e5d92bda6634463a9c634d38a1aa4b6dab336da5c52e884cbde95",
+      url = "https://dl.google.com/closure-compiler/compiler-latest.zip",


Is this safe? Wouldn't the build break if a new "latest" version is uploaded?

Oh yeah, good catch. This should reference -20160208 which is the latest release I can find.

I'll submit an update to this separately.

Good catch. I actually thought about that but then I saw https://github.com/bazelbuild/bazel/blob/master/tools/build_rules/closure/closure_repositories.bzl#L103 :(

Since Closure Compiler releases come more often, we should not use "latest". I didn't find a versioned URL for Closure Templates though so we'd have to keep it as is for now.

damienmg · 2016-02-17T22:21:51Z

@Dominator008: I prefer gerrit personally but we accept both PR and gerrit review request, it's just we don't merge with the merge button because we have to merge it on the trunk inside google

jart · 2016-02-17T22:22:02Z

tools/build_rules/closure/closure_js_binary.bzl

@@ -46,6 +48,21 @@ _COMPILATION_LEVELS = {
  "advanced": ["--compilation_level=ADVANCED"]
 }

+_SUPPORTED_LANGUAGES = {
+  "es3": ["ES3"],


Why support name aliases? Why not require that users enter the canonical name documented by the Closure Compiler CLI?

--language_in VAL : Sets what language spec that input sources conform. Options: ECMASCRIPT3, ECMASCRIPT5, ECMASCRIPT5_STRICT, ECMASCRIPT6 (default), ECMASCRIPT6_STR ICT, ECMASCRIPT6_TYPED (experimental)

The CLI accepts name aliases.

It's undocumented behavior though. Are the Bazel devs OK with propagating undocumented (or possibly deprecated) behavior to new code?

Would you mind updating the documentation / help message within Closure Compiler? :)

…ary() Update Closure Compiler to v20160208 Update Closure Library to v20160208 Update Closure Stylesheets to 1.1.0 Add "--language_in" and "--language_out" to closure_js_binary.bzl With these flags users can transpile their closure_js_binary() from ES6 to ES3. Change "--manage_closure_dependencies" to "--dependency_mode=LOOSE" because the former has been deprecated in Closure Compiler v20160208. -- Reviewed-on: #910 MOS_MIGRATED_REVID=114898222

Update dependencies in closure_repositories.bzl

af1d67f

Update Closure Compiler to v20160208 Update Closure Library to v20160208 Update Closure Stylesheets to 1.1.0

googlebot added the cla: yes label Feb 16, 2016

kjiwa reviewed Feb 17, 2016
View reviewed changes

Add "--language_in" and "--language_out" to closure_js_binary.bzl

33573dc

With these flags users can transpile their closure_js_binary() from ES6 to ES3. Change "--manage_closure_dependencies" to "--dependency_mode=LOOSE" because the former has been deprecated in Closure Compiler v20160208.

kjiwa added a commit that referenced this pull request Feb 17, 2016

Merge pull request #910 from Dominator008/closure

3f70577

Update Closure Tools dependencies and add language arguments to closure_js_binary()

kjiwa merged commit 3f70577 into bazelbuild:master Feb 17, 2016

Dominator008 mentioned this pull request Feb 17, 2016

Publish new releases to Maven Central google/closure-compiler#1485

Closed

Dominator008 deleted the closure branch February 17, 2016 21:23

damienmg mentioned this pull request Feb 17, 2016

Revert "Update Closure Tools dependencies and add arguments to closure_js_binary()" #924

Closed

bazel-io pushed a commit that referenced this pull request Feb 17, 2016

Revert "Merge pull request #910 from Dominator008/closure"

cf521dd

This reverts commit 3f70577, reversing changes made to cbbbf2e. Accidental merge, merged pull request have to go through googlesource first

jart reviewed Feb 17, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Closure Tools dependencies and add arguments to closure_js_binary() #910

Update Closure Tools dependencies and add arguments to closure_js_binary() #910

Dominator008 commented Feb 16, 2016

damienmg commented Feb 16, 2016

damienmg commented Feb 17, 2016

kjiwa Feb 17, 2016

Dominator008 Feb 17, 2016

kjiwa Feb 17, 2016

Dominator008 Feb 17, 2016

damienmg Feb 17, 2016

kjiwa commented Feb 17, 2016

kjiwa commented Feb 17, 2016

damienmg commented Feb 17, 2016

damienmg commented Feb 17, 2016

damienmg commented Feb 17, 2016

Dominator008 commented Feb 17, 2016

jart Feb 17, 2016

kjiwa Feb 17, 2016

kjiwa Feb 17, 2016

Dominator008 Feb 17, 2016

damienmg commented Feb 17, 2016

jart Feb 17, 2016

Dominator008 Feb 17, 2016

jart Feb 17, 2016

Dominator008 Feb 17, 2016

Update Closure Tools dependencies and add arguments to closure_js_binary() #910

Update Closure Tools dependencies and add arguments to closure_js_binary() #910

Conversation

Dominator008 commented Feb 16, 2016

damienmg commented Feb 16, 2016

damienmg commented Feb 17, 2016

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

kjiwa commented Feb 17, 2016

kjiwa commented Feb 17, 2016

damienmg commented Feb 17, 2016

damienmg commented Feb 17, 2016

damienmg commented Feb 17, 2016

Dominator008 commented Feb 17, 2016

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

damienmg commented Feb 17, 2016

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