From 6c8d8d62a56ddc7cf8ad47b2902ebfe072d7b25e Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Fri, 22 Jul 2022 14:57:39 -0400 Subject: [PATCH 1/9] Use esbuild for tb_webapp_binary (prod and dev). --- WORKSPACE | 4 +++ package.json | 1 + tensorboard/webapp/BUILD | 35 ++++++++++++------- tensorboard/webapp/dev_assets/BUILD | 13 +++++-- .../webapp/index_polymer3.uninlined.html | 3 +- yarn.lock | 5 +++ 6 files changed, 45 insertions(+), 16 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 74c23b7a9b..ec1b897e02 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -88,6 +88,10 @@ yarn_install( yarn_lock = "//:yarn.lock", ) +load("@build_bazel_rules_nodejs//toolchains/esbuild:esbuild_repositories.bzl", "esbuild_repositories") + +esbuild_repositories(npm_repository = "npm") + http_archive( name = "io_bazel_rules_sass", sha256 = "ee6d527550d42af182673c3718da98bb9205cabdeb08eacc0e3767fa3f2b051a", diff --git a/package.json b/package.json index f786138816..c8c2aa8659 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "@angular/compiler": "^12.2.0", "@angular/compiler-cli": "^12.2.0", "@bazel/concatjs": "^4.6.1", + "@bazel/esbuild": "^4.6.2", "@bazel/ibazel": "^0.15.9", "@bazel/jasmine": "^4.6.1", "@bazel/rollup": "^4.6.1", diff --git a/tensorboard/webapp/BUILD b/tensorboard/webapp/BUILD index 6b54a65be3..b1687c1380 100644 --- a/tensorboard/webapp/BUILD +++ b/tensorboard/webapp/BUILD @@ -1,6 +1,7 @@ load("//tensorboard/defs:defs.bzl", "tf_external_sass_libray", "tf_js_binary", "tf_ng_module", "tf_ng_web_test_suite", "tf_sass_binary", "tf_svg_bundle", "tf_ts_library") load("//tensorboard/defs:js.bzl", "tf_resource_digest_suffixer") load("//tensorboard/defs:web.bzl", "tb_combine_html", "tf_web_library") +load("@npm//@bazel/esbuild:index.bzl", "esbuild") package(default_visibility = ["//tensorboard:internal"]) @@ -171,22 +172,29 @@ tf_ng_module( ], ) -# Compile the prepared Angular app to a JS binary. -tf_js_binary( +# BDTODO: Document the fact that bazel only has first-class support for +# esbuild bundling. Examples (find one online) suggest using it with ts_project +# but we were able to make it work with ts_library. +esbuild( name = "tb_webapp_binary", - compile = 1, entry_point = "main_prod.ts", + # BDTODO: Document why we can't use "esm" (d3's dispatchEvent placed into + # global scope conflicts with window.dispathEvent) + # BDTODO: Also note that we could use "esm" with minify since names are + # largely mangled but given the small possibility of generating a + # global-scope conflict, and given the very very small saving in size (11 bytes), we + # do iife anyways. + format="iife", + minify=True, deps = [ - ":ng_main", - "//tensorboard/webapp/angular:expect_angular_material_tabs", - "//tensorboard/webapp/angular:expect_angular_material_toolbar", - "@npm//@angular/common", - "@npm//@angular/core", - "@npm//@angular/platform-browser", - "@npm//@ngrx/store", - "@npm//rxjs", - "@npm//zone.js", + ":ng_main", ], + args = { + # For compatibility with ts_library, must specify that 'mjs' extensions are + # preferred, since that is where es2015/esm code is generated by ts_library. + # https://github.com/bazelbuild/rules_nodejs/issues/2691#issuecomment-846429871 + "resolveExtensions": [".mjs", ".js"] + }, ) # Wrap the Angular app JS binary as a library. @@ -221,7 +229,8 @@ tf_web_library( deps = [ ":tb_webapp", "//tensorboard/components/tf_imports:roboto", - "//tensorboard/webapp/widgets/source_code/monaco:requirejs", + # BDTODO: Is this needed? For debugger_v2? +# "//tensorboard/webapp/widgets/source_code/monaco:requirejs", ], ) diff --git a/tensorboard/webapp/dev_assets/BUILD b/tensorboard/webapp/dev_assets/BUILD index eb603795ba..da4c242fae 100644 --- a/tensorboard/webapp/dev_assets/BUILD +++ b/tensorboard/webapp/dev_assets/BUILD @@ -8,6 +8,7 @@ # alias asset, we would not need the enitrely different Bazel package. load("//tensorboard/defs:defs.bzl", "tf_dev_js_binary", "tf_js_binary", "tf_ng_module") load("//tensorboard/defs:web.bzl", "tf_web_library") +load("@npm//@bazel/esbuild:index.bzl", "esbuild") package(default_visibility = ["//tensorboard:__pkg__"]) @@ -24,12 +25,20 @@ tf_ng_module( ], ) -tf_dev_js_binary( +esbuild( name = "tb_webapp_binary", entry_point = "main_dev.ts", + format="iife", + minify=False, deps = [ - ":ng_main", + ":ng_main", ], + args = { + # For compatibility with ts_library, must specify that 'mjs' extensions are + # preferred, since that is where es2015/esm code is generated by ts_library. + # https://github.com/bazelbuild/rules_nodejs/issues/2691#issuecomment-846429871 + "resolveExtensions": [".mjs", ".js"] + }, ) tf_web_library( diff --git a/tensorboard/webapp/index_polymer3.uninlined.html b/tensorboard/webapp/index_polymer3.uninlined.html index f5421bb0ae..168957b27f 100644 --- a/tensorboard/webapp/index_polymer3.uninlined.html +++ b/tensorboard/webapp/index_polymer3.uninlined.html @@ -63,7 +63,8 @@

TensorBoard requires JavaScript

Please enable JavaScript and reload this page.

- + + - + + + From 90f6e5da905b6ea6e6c80ce2b10512c40a0d3819 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Thu, 28 Jul 2022 16:10:19 -0400 Subject: [PATCH 5/9] Closer to done. --- package.json | 6 -- tensorboard/defs/BUILD | 5 - tensorboard/defs/defs.bzl | 56 ++++------- tensorboard/defs/internal/BUILD | 2 + tensorboard/defs/internal/js.bzl | 149 ---------------------------- tensorboard/defs/rollup_config.js | 56 ----------- tensorboard/defs/terser_config.json | 18 ---- yarn.lock | 126 ++--------------------- 8 files changed, 30 insertions(+), 388 deletions(-) delete mode 100644 tensorboard/defs/internal/js.bzl delete mode 100644 tensorboard/defs/rollup_config.js delete mode 100644 tensorboard/defs/terser_config.json diff --git a/package.json b/package.json index c8c2aa8659..940ee17bf0 100644 --- a/package.json +++ b/package.json @@ -35,11 +35,7 @@ "@bazel/esbuild": "^4.6.2", "@bazel/ibazel": "^0.15.9", "@bazel/jasmine": "^4.6.1", - "@bazel/rollup": "^4.6.1", - "@bazel/terser": "^4.6.1", "@bazel/typescript": "^4.6.1", - "@rollup/plugin-commonjs": "^20.0.0", - "@rollup/plugin-node-resolve": "^13.0.4", "@types/d3": "5.7.2", "@types/jasmine": "^3.8.2", "@types/lodash": "^4.14.172", @@ -59,8 +55,6 @@ "prettier": "2.4.1", "prettier-plugin-organize-imports": "2.3.4", "requirejs": "^2.3.6", - "rollup": "^2.56.2", - "terser": "^5.14.2", "tslib": "^2.3.0", "typescript": "4.5.4", "yarn-deduplicate": "^5.0.0" diff --git a/tensorboard/defs/BUILD b/tensorboard/defs/BUILD index f6d8e83a0e..4cfb61ed28 100644 --- a/tensorboard/defs/BUILD +++ b/tensorboard/defs/BUILD @@ -42,11 +42,6 @@ tb_proto_library( ], ) -exports_files([ - "rollup_config.js", - "terser_config.json", -]) - ts_library( name = "strict_types", srcs = ["strict_type_check.d.ts"], diff --git a/tensorboard/defs/defs.bzl b/tensorboard/defs/defs.bzl index 79a3a3c896..d4b60b0046 100644 --- a/tensorboard/defs/defs.bzl +++ b/tensorboard/defs/defs.bzl @@ -13,23 +13,10 @@ # limitations under the License. """External-only delegates for various BUILD rules.""" -# BDTODO: Delete. -load("@npm//@bazel/rollup:index.bzl", "rollup_bundle") load("@npm//@bazel/concatjs:index.bzl", "karma_web_test_suite") load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_library") load("@io_bazel_rules_sass//:defs.bzl", "npm_sass_library", "sass_binary", "sass_library") -# BDTODO: Delete. -load("@npm//@bazel/terser:index.bzl", "terser_minified") load("@npm//@bazel/esbuild:index.bzl", "esbuild") -# BDTODO: Delete. -load("//tensorboard/defs/internal:js.bzl", _tf_dev_js_binary = "tf_dev_js_binary") - -# BDTODO: Delete the old rollup config. Delete rollup from the project entirely (remove from WORKSPACE and elsewhere?) - -# BDTODO: Delete the definition of _tf_dev_js_binary. - -# BDTODO: Delete. -tf_dev_js_binary = _tf_dev_js_binary def tensorboard_webcomponent_library(**kwargs): """Rules referencing this will be deleted from the codebase soon.""" @@ -43,32 +30,30 @@ def tf_js_binary( **kwargs): """Rules for creating a JavaScript bundle. - BDTODO: Revisit documentation and the choice of properties. - Args: name: Name of the target. compile: whether to compile when bundling. Only used internally. dev_mode_only: whether the binary is for development. When True, it will - omit the Terser. + omit the minification step. includes_polymer: whether this binary contains Polymer. Only used internally. - **kwargs: keyword arguments to rollup_bundle. Please refer to - https://bazelbuild.github.io/rules_nodejs/Built-ins.html#rollup_bundle - for more details. - BDTODO: Used for entry_point, deps, visibility + **kwargs: Other keyword arguments to esbuild(). Typically used for + entry_point, deps, and visibility. Please refer to + https://esbuild.github.io/api/ for more details. """ - # BDTODO: Document the fact that bazel only has first-class support for - # esbuild bundling. Examples (find one online) suggest using it with ts_project - # but we were able to make it work with ts_library. + # Bazel's integration with esbuild is limited to bundling. Bazel documents + # how to use esbuild with ts_project[1] but we use ts_library instead of + # ts_project. We've managed to get esbuild working with ts_library but its + # support is tenuous. + # + # [1]: (https://www.npmjs.com/package/@bazel/esbuild) esbuild( name = name, - # BDTODO: Document why we can't use "esm" (d3's dispatchEvent placed into - # global scope conflicts with window.dispathEvent) - # BDTODO: Also note that we could use "esm" with minify since names are - # largely mangled but given the small possibility of generating a - # global-scope conflict, and given the very very small saving in size (11 bytes), we - # do iife anyways. + # Use "iife" format instead of "esm" because "esm" writes symbols at + # the global level and tends to overwrite `window` functions. "iife" is + # just a thing wrapper around "esm" (it adds 11 bytes) and doesn't + # suffer from the same problem. format="iife", minify= False if dev_mode_only else True, args = { @@ -77,12 +62,13 @@ def tf_js_binary( # https://github.com/bazelbuild/rules_nodejs/issues/2691#issuecomment-846429871 # BDTODO: Can we be rid of this with usage of mainFields? "resolveExtensions": [".mjs", ".js"], - # BDTODO: Can we document who is using each main field? - # BDTODO: Document the fact that we don't really know the best order - # here. We picked this up from the "old" rollup config. Note that the - # default values definitely don't work (for some reason some deps, like - # d3, give us the "node" version of their files that require the - # node version of xmlhttprequest) + # The reasoning for these particular mainFields values are lost to + # history. These come from the old rollup bundler configuration. + # We do know that the esbuild default values for mainFields don't + # work for us. In particular we ran into problems with + # esbuild pulling in browser-unfriendly versions of some libraries. + # For example, the "node" version of d3 transitively pulls in the + # node implementation of XmlHttpRequest. "mainFields": ["browser", "es2015", "module", "jsnext:main", "main"], }, **kwargs diff --git a/tensorboard/defs/internal/BUILD b/tensorboard/defs/internal/BUILD index 9f57cd8427..ee131f7c2b 100644 --- a/tensorboard/defs/internal/BUILD +++ b/tensorboard/defs/internal/BUILD @@ -2,6 +2,8 @@ package(default_visibility = ["//tensorboard:internal"]) licenses(["notice"]) +# BDTODO: Can we delete these other things? + py_binary( name = "resource_digest_suffixer", srcs = ["resource_digest_suffixer.py"], diff --git a/tensorboard/defs/internal/js.bzl b/tensorboard/defs/internal/js.bzl deleted file mode 100644 index 88d1e79855..0000000000 --- a/tensorboard/defs/internal/js.bzl +++ /dev/null @@ -1,149 +0,0 @@ -# Copyright 2021 The TensorFlow Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""JavaScript related TensorBoard build rules.""" - -load("@bazel_skylib//lib:paths.bzl", "paths") -load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo", "NpmPackageInfo", "node_modules_aspect") - -def _tf_dev_js_binary_impl(ctx): - files_depsets = [] - - bootstrap_and_deps = ctx.attr._ambient_deps + ctx.attr.deps - for dep in bootstrap_and_deps: - if JSNamedModuleInfo in dep: - # Collect UMD modules compiled by tf_ts_library - files_depsets.append(dep[JSNamedModuleInfo].sources) - elif NpmPackageInfo not in dep and hasattr(dep, "files"): - # Collect manually specified files or File from npm dependencies. It omits - # package.json (i.e., ones in `NpmPackageInfo`). - files_depsets.append(dep.files) - - for target in ctx.attr._anonymous_umd_deps: - file = target.files.to_list()[0] - module_name = ctx.attr._anonymous_umd_deps[target] - named_file = ctx.actions.declare_file(file.path + ".named_umd.js") - - # Patch anonymous umd modules to have named in their declarations. For instance, - # it converts `define(['exports'], ...` to `define('d3', ['exports'], ...`. - # `define`'s argument behaves differently based on arity. For instance: - # 1: expects the argument to be a factory function to be invoked. Anonymous and - # no dependency. - # 2: expects an array then a function. First arguments define dependencies to be - # injected into the factory. Anonymous with dependencies. - # 3: expects string, an array, then, a function. First argument is name of the - # module. Named module with deps. - ctx.actions.expand_template( - template = file, - output = named_file, - substitutions = { - # d3, three, umap-js, and tfjs - "define([": "define('%s', [" % module_name, - # Lodash - "define(function()": "define('%s', function()" % module_name, - # Zone.js - "define(factory": "define('%s', factory" % module_name, - }, - is_executable = False, - ) - files_depsets.append(depset([named_file])) - - files = depset(transitive = files_depsets) - - file_list = files.to_list() - - concat_command = """ - output="$1" && shift - entry_point="$1" && shift - { - awk 'BEGINFILE { print "// file: " FILENAME } { print }' "$@" - printf ';require(["%s"]);\n' "${entry_point}" - } >"${output}" - """ - - entry_point_module_name = _get_module_name(ctx, ctx.file.entry_point) - - concat_args = ( - [ctx.outputs.js.path, entry_point_module_name] + - [file.path for file in file_list] - ) - - ctx.actions.run_shell( - mnemonic = "ConcatJs", - progress_message = "concatenating JavaScript files from dependencies", - inputs = file_list, - outputs = [ctx.outputs.js], - command = concat_command, - arguments = concat_args, - ) - -def _get_module_name(ctx, entry_point_file): - path_without_ext = paths.replace_extension(entry_point_file.short_path, "") - return ctx.workspace_name + "/" + path_without_ext - -tf_dev_js_binary = rule( - _tf_dev_js_binary_impl, - attrs = { - "deps": attr.label_list( - aspects = [node_modules_aspect], - doc = """Targets that produce JavaScript, such as tf_ts_library, are - dependencies of the application.""", - mandatory = True, - ), - "entry_point": attr.label( - allow_single_file = [".ts"], - doc = """A module that should be executed as script gets parsed. Generally - entry to the application.""", - mandatory = True, - ), - # Due to the nature of Angular and certain libraries, they assume presence of - # library in the bundle. Dependencies appearing in `_ambient_deps` are loaded - # before the `deps`. - "_ambient_deps": attr.label_list( - default = [ - "@npm//:node_modules/requirejs/require.js", - ":common_umd_lib", - "@npm//:node_modules/reflect-metadata/Reflect.js", - "@npm//:node_modules/@angular/localize/bundles/localize-init.umd.js", - ], - allow_files = True, - ), - # Libraries like d3 and lodash export UMD compatible bundled in their node_modules - # but they are using "anonymous module" of requirejs. Anonymous module is where - # you define a module without a name (e.g., `define(factory) or - # `define([dep1], factory)`). They are often intended to be loaded via - # ` - - + From 08fb3fcc2628d9bdcf6d68d3c2b98734636a3174 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Fri, 29 Jul 2022 06:47:52 -0400 Subject: [PATCH 8/9] Another cleanup pass. --- WORKSPACE | 2 ++ tensorboard/defs/defs.bzl | 35 +++++++++++-------- .../widgets/line_chart_v2/lib/worker/BUILD | 2 +- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index ec1b897e02..1773c250c4 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -88,6 +88,8 @@ yarn_install( yarn_lock = "//:yarn.lock", ) +# Load esbuild rules for bazel. +# https://bazelbuild.github.io/rules_nodejs/esbuild.html load("@build_bazel_rules_nodejs//toolchains/esbuild:esbuild_repositories.bzl", "esbuild_repositories") esbuild_repositories(npm_repository = "npm") diff --git a/tensorboard/defs/defs.bzl b/tensorboard/defs/defs.bzl index 58b61e3105..2112e72746 100644 --- a/tensorboard/defs/defs.bzl +++ b/tensorboard/defs/defs.bzl @@ -13,10 +13,11 @@ # limitations under the License. """External-only delegates for various BUILD rules.""" -load("@npm//@bazel/concatjs:index.bzl", "karma_web_test_suite") -load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_library") load("@io_bazel_rules_sass//:defs.bzl", "npm_sass_library", "sass_binary", "sass_library") +load("@npm//@bazel/concatjs:index.bzl", "karma_web_test_suite") load("@npm//@bazel/esbuild:index.bzl", "esbuild") +load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_library") + def tensorboard_webcomponent_library(**kwargs): """Rules referencing this will be deleted from the codebase soon.""" @@ -44,33 +45,37 @@ def tf_js_binary( for more details. """ - # Bazel's integration with esbuild is limited to bundling. Bazel documents - # how to use esbuild with ts_project[1] but we use ts_library instead of - # ts_project. We've managed to get esbuild working with ts_library but its - # support is tenuous. + # esbuild is a fast JavaScript bundler[1] appropriate for both production + # and development builds. + # + # Bazel documents[2] how to use esbuild bundling with ts_project but we use + # the not-quite-deprecated ts_library rule instead of ts_project. We've + # managed to get esbuild working with ts_library but its long-term support + # is unknown. # - # [1]: (https://www.npmjs.com/package/@bazel/esbuild) + # [1]: https://esbuild.github.io/ + # [2]: https://www.npmjs.com/package/@bazel/esbuild esbuild( name = name, visibility = visibility, # Use "iife" format instead of "esm" because "esm" writes symbols at # the global level and tends to overwrite `window` functions. "iife" is - # just a thing wrapper around "esm" (it adds 11 bytes) and doesn't - # suffer from the same problem. + # just a thin wrapper around "esm" (it adds 11 bytes) and doesn't + # suffer from the same overwriting problem. format="iife", minify= False if dev_mode_only else True, args = { - # For compatibility with ts_library, must specify that 'mjs' extensions are - # preferred, since that is where es2015/esm code is generated by ts_library. + # Must specify that 'mjs' extensions are preferred, since that is + # the extension that is used for es2015/esm code generated by + # ts_library. # https://github.com/bazelbuild/rules_nodejs/issues/2691#issuecomment-846429871 "resolveExtensions": [".mjs", ".js"], # The reasoning for these particular mainFields values are lost to # history. These come from the old rollup bundler configuration. - # We do know that the esbuild default values for mainFields don't + # We do know that the esbuild default values for mainFields do not # work for us. In particular we ran into problems with - # esbuild pulling in browser-unfriendly versions of some libraries. - # For example, the "node" version of d3 transitively pulls in the - # node implementation of XmlHttpRequest. + # esbuild pulling in "node"-specific versions of some libraries that + # are incompatible with browsers. "mainFields": ["browser", "es2015", "module", "jsnext:main", "main"], }, **kwargs diff --git a/tensorboard/webapp/widgets/line_chart_v2/lib/worker/BUILD b/tensorboard/webapp/widgets/line_chart_v2/lib/worker/BUILD index e38f2d2309..a049373212 100644 --- a/tensorboard/webapp/widgets/line_chart_v2/lib/worker/BUILD +++ b/tensorboard/webapp/widgets/line_chart_v2/lib/worker/BUILD @@ -92,7 +92,7 @@ tf_js_binary( entry_point = "worker_chart_bridge.ts", visibility = ["//tensorboard:internal"], deps = [ - ":worker_chart_bridge", + ":worker_chart_bridge", ], ) From 750e4021193e5f8ace317811353ef084966fa4b0 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Fri, 29 Jul 2022 07:27:53 -0400 Subject: [PATCH 9/9] buildifier. --- tensorboard/webapp/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/webapp/BUILD b/tensorboard/webapp/BUILD index 0ada3fd2a9..afbc992c36 100644 --- a/tensorboard/webapp/BUILD +++ b/tensorboard/webapp/BUILD @@ -177,7 +177,7 @@ tf_js_binary( compile = 1, entry_point = "main_prod.ts", deps = [ - ":ng_main", + ":ng_main", ], )