-
Notifications
You must be signed in to change notification settings - Fork 1.7k
core: introduce rules_nodejs and angular shim #2472
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
Conversation
TensorBoard has been using custom Vulcanization to build the frontend but we have plans to use better officially Bazel-team supported rule: rules_nodejs. This adds the WORKSPACE dependency to the repo. Also, to prove the correctness of the integration, this change also Angular CLI generated (heavily trimmed down and modified) code with its build rule.
| @@ -0,0 +1,18 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note that, by default, ts_library and necessary rule use "//:tsconfig.json" as the default label for tsconfig field. https://github.com/bazelbuild/rules_nodejs/blob/432f88c8acc47b95ae6f368d3c5b7fe3e65baef7/packages/typescript/src/internal/build_defs.bzl#L424-L425
package.json
Outdated
| @@ -0,0 +1,49 @@ | |||
| { | |||
| "name": "tensorboard", | |||
| "version": "1.14.0", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably we'll want some way to keep this number, at a minimum, in sync with our pip package version? Maybe we can add a presubmit check that verifies that this number always matches version.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that or we can make this version preposterous like 0. This number is mostly useful only when we are publishing a npm package which we are not (note, private: "true"). Which would you prefer?
| # Keep this version in sync with the BAZEL environment variable defined | ||
| # in our .travis.yml config. | ||
| versions.check(minimum_bazel_version = "0.22.0") | ||
| versions.check(minimum_bazel_version = "0.26.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, is 0.26.1 the minimum for any reasonable version of rules_nodejs? Or if we went back a few versions on that, could we retain compatibility across more than one bazel version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, is 0.26.1 the minimum for any reasonable version of rules_nodejs?
0.26.0 is the minimum version required by rules_nodejs.
| exports_files(["tsconfig.json"]) | ||
|
|
||
|
|
||
| ts_config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this level of the Bazel package hierarchy doesn't exist internally, do we know how we're going to sync this and the other files? Or do we not use any of these internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we not use any of these internally?
This :)
|
Thanks for working on this, @stephanwlee . cc'ing myself for notifications. |
|
Okay, now, I am confident that everything will work as intended. I tried to transfer the new addition internally and I did get everything to build correctly! cl/260512283 |
|
Here is a small doc on how to manage dependency in the new world. #2003 (comment) I will update it and keep it up-to-date. |
nfelt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Onwards into the bright new Angular dawn! 📐 🌄
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we perhaps set this to "0.0.0-unused" just to be extra clear?
|
🎉 |
|
💄 ? |
| "@angular/cli": "^8.1.2", | ||
| "@angular/compiler": "^8.1.2", | ||
| "@angular/compiler-cli": "^8.1.2", | ||
| "@bazel/bazel": "^0.23.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR bumps the Bazel version in WORKSPACE from 0.22.0 to 0.26.1,
but adds a dependency in package.json at version 0.23.2. Is there any
reason for the discrepancy?
Cf. #2534 (comment)

TensorBoard has been using custom Vulcanization to build the frontend
but we have plans to use better officially Bazel-team supported rule:
rules_nodejs. This adds the WORKSPACE dependency to the repo.
Also, to prove the correctness of the integration, this change also
Angular CLI generated (heavily trimmed down and modified) code with its
build rule.