Zone bazel by JiaLiPassion · Pull Request #1232 · angular/zone.js

JiaLiPassion · 2019-05-07T16:36:30Z

Bazel rule for zone.js

googlebot · 2019-05-07T16:36:54Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-07T16:36:55Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-07T19:03:17Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-07T19:03:18Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

alexeagle · 2019-05-07T19:04:04Z

+        "@bazel/bazel",
+        "@bazel/ibazel",
+        "@bazel/buildifier",
+        # TODO: fails because it tries to install itself elsewhere on disk:


remove unneeded comments

done, thanks!

alexeagle · 2019-05-07T19:05:06Z

+* found in the LICENSE file at https://angular.io/license
+*/
+!function(t){"function"==typeof define&&define.amd?define(t):t()}(function(){"use strict";
+/**


seems like we should figure out how to avoid having the license duplicated? I suppose it compresses well...

The current zone.js bundles have the same issues, because inside the source code, we also have the license header, so I think we can resolve this issue later.

alexeagle · 2019-05-07T19:06:16Z

+package(default_visibility = ["//:__pkg__"])
+
+exports_files([
+    "zone.ts",


exposing sources outside the package is a smell, usually any rules using them should be declared here too. At least add a comment explaining why another package needs to be able to use these sources?
(the .ts files in particular seem wrong here, they should be compiled by a rule in this package)

This for test, I have removed it and will re-design when we figure out how to make karma for zone.js work.

alexeagle · 2019-05-07T19:07:39Z

@@ -0,0 +1,12 @@
+load("//lib:rollup.bzl", "rollup_bundle")
+def rollup(bundles):


I don't think the extra indirection to this file is useful, just inline this into the BUILD.bazel ?

Sure, you are right, I have removed all separated rollup.bzl and move the definition into BUILD.bazel.

alexeagle · 2019-05-07T19:08:34Z

+  ],
+  srcs = glob([
+    "**/*.spec.ts",
+    "browser_entry_point.ts",


Once we move this to angular/angular, buildifier will warn that entries in a glob with no wildcard should be outside the glob
glob(["**/*.spec.ts"]) + ["browser-zone-setup.ts"]

Got it, I will modify this later, because currently this settings not work... I will delete all bazel settings of test for now. So this PR is only about npm_package.

alexeagle · 2019-05-07T19:10:40Z

+    "test/main",
+]
+
+def get_web_test_deps():


this isn't called anywhere yet?

We generally prefer to just expand this out into the BUILD file. It's a bit repetitive to have 5 ts_library targets with similar setup, however tooling like ts_auto_deps makes it manageable. By having this in a macro, tooling like buildozer will no longer be able to machine-edit the rules

Got it, I will modify this later, because currently this settings not work... I will delete all bazel settings of test for now. So this PR is only about npm_package.

alexeagle · 2019-05-07T19:13:03Z

+    cmd = "cp $< $(@D)/zone_externs.js",
+)
+
+npm_package(


we should add an e2e test for the structure and content of the package so that the genrule spaghetti above can be safely refactored out.
something like https://github.com/angular/angular/blob/master/packages/bazel/test/ng_package/core_package.spec.ts maybe?

Sure, got it, I have added test cases.

alexeagle · 2019-05-07T19:15:25Z

+    for b in ES2015_BUNDLES
+]
+
+# Extract and rename each es5 bundle to a .js and .min.js in the dist/ dir


this seems wrong. If we want the artifacts to appear in the dist/ subfolder in the output tree (and therefore in the npm package) then we should probably have a BUILD file in dist/ in the source tree which declares them. then we don't have to move things around. I can do some refactoring to help with that.

Got it, I made it work, and testing it now.

googlebot · 2019-05-14T02:28:31Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-14T02:28:32Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-15T14:38:27Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-15T14:38:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alexeagle

Yay! this looks really good

alexeagle · 2019-05-21T03:06:10Z

+    "closure/zone_externs.js",
+])
+
+filegroup(


I don't think we need both the exports_files and the filegroup to expose the zone_externs.js file

alexeagle · 2019-05-21T03:07:56Z

 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.io/license
 */
+/// <amd-module name="__main__/lib/zone" />


this is a bit gross, users don't want to see main - in angular we do something to trim these lines from published .d.ts

Got it, could you show me the source that how angular trim those lines? Thanks

alexeagle · 2019-05-21T03:09:00Z

+        "package.json",
+    ],
+    replacements = {
+        # Since the bazel package is experimental, patch the version number


this seems like copy-paste - this isn't the bazel package

You are right, I removed this replacements.

alexeagle · 2019-05-21T03:10:08Z

+
+genrule(
+    name = "zone_d_ts",
+    srcs = ["//lib"],


is this concatenating multiple .d.ts inputs into a single output? maybe we should use the api-extractor like we do in the angular repo.

This rule just copy the zone.d.ts from lib to dist/zone.js.d.ts, we only have one d.ts file here.

alexeagle · 2019-05-21T03:10:46Z

+
+# Extract and rename each es5 bundle to a .js and .min.js in the dist/ dir
+[
+    genrule(


this is pretty gross... maybe I can find a way to refactor it later

You are right, thanks!

alexeagle · 2019-05-21T03:11:31Z

+    srcs = [":npm_package_spec_lib"],
+    data = [
+        ":npm_package",
+        "@npm//@types/shelljs",


I don't think the typings are needed by the test, these would only be useful for a rule that does type-checking

Got it, typings removed. thanks!

JiaLiPassion · 2019-05-22T16:33:45Z

angular/angular#30615 passed, so this PR should work fine at least in angular repo. Need to very it in G3 too.

googlebot added the cla: no label May 7, 2019

JiaLiPassion force-pushed the zone-bazel branch from 82008e1 to 0bfc70f Compare May 7, 2019 16:39

alexeagle added cla: yes and removed cla: no labels May 7, 2019

alexeagle reviewed May 7, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels May 14, 2019

JiaLiPassion force-pushed the zone-bazel branch 3 times, most recently from b363288 to 525ad67 Compare May 15, 2019 14:38

googlebot added cla: yes and removed cla: no labels May 15, 2019

JiaLiPassion force-pushed the zone-bazel branch from 525ad67 to 0da331f Compare May 15, 2019 15:02

alexeagle approved these changes May 21, 2019

View reviewed changes

JiaLiPassion force-pushed the zone-bazel branch from 0da331f to 40b8219 Compare May 21, 2019 23:57

JiaLiPassion mentioned this pull request May 22, 2019

WIP: test zone-bazel dist angular/angular#30615

Closed

JiaLiPassion force-pushed the zone-bazel branch from 40b8219 to 3abc99e Compare May 22, 2019 16:09

JiaLiPassion force-pushed the zone-bazel branch from 3abc99e to 3b121ac Compare May 22, 2019 16:48

Build: Use bazel to build and package zone.js

54c5038

JiaLiPassion force-pushed the zone-bazel branch from 3b121ac to 54c5038 Compare May 22, 2019 23:11

alexeagle merged commit 71b9371 into angular:master May 31, 2019

		@@ -0,0 +1,12 @@
		load("//lib:rollup.bzl", "rollup_bundle")
		def rollup(bundles):

Conversation

JiaLiPassion commented May 7, 2019

Uh oh!

googlebot commented May 7, 2019

Uh oh!

googlebot commented May 7, 2019

Uh oh!

googlebot commented May 7, 2019

Uh oh!

googlebot commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented May 14, 2019

Uh oh!

googlebot commented May 14, 2019

Uh oh!

googlebot commented May 15, 2019

Uh oh!

googlebot commented May 15, 2019

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented May 22, 2019

Uh oh!

JiaLiPassion May 14, 2019 •

edited

Loading