-
Notifications
You must be signed in to change notification settings - Fork 1.7k
infra: remove JSCompiler from Vulcanize #4906
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 no longer uses JSCompiler in a meaningful way. For the most TypeScript/JavaScript source, we use rollup via rules_nodejs. Hence, we now remove the JSCompiler portion from the Vulcanization. Soon after, vulcanize.bzl will be completely removed accordingly.
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.
Thanks for doing this!
| """Rule for building the HTML binary.""" | ||
|
|
||
| load("@io_bazel_rules_closure//closure:defs.bzl", "closure_js_aspect") | ||
| load("@io_bazel_rules_closure//closure/private:defs.bzl", "collect_js", "long_path", "unfurl") # buildifier: disable=bzl-visibility |
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.
# buildifier: disable=bzl-visibility lol
KEEP OUT or don't I'm a sign not a cop
tensorboard/defs/internal/html.bzl
Outdated
| ) | ||
|
|
||
| # vulcanize | ||
| ignore_regexs_file_set = depset() |
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 looks unused?
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.
Removed. Thanks.
tensorboard/defs/internal/html.bzl
Outdated
| attrs = { | ||
| "input_path": attr.string(mandatory = True), | ||
| "output_path": attr.string(mandatory = True), | ||
| # If specified, it extracts scripts into {name}.js and inserts <script src="{js_path}">. |
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.
Could we use doc parameters for these attrs to provide a brief description of each one, just for posterity so we can better understand how this is used? They can just be a single line.
One thing that I had to go read Vulcanize.java to remember is the distinction between input_path and deps - i.e. that input_path is where we start the crawl from, and deps has to contain all the webfiles that input_path is referencing. Just brief mentions of things like that would help contextualize what's happening here.
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.
I gave it a stab. Thanks for the suggestion.
|
Thanks, LGTM! |
TensorBoard no longer uses JSCompiler in a meaningful way. For the most
TypeScript/JavaScript source, we use rollup via rules_nodejs. Hence, we
now remove the JSCompiler portion from the Vulcanization.
Soon after, vulcanize.bzl will be completely removed accordingly.