From 1601ee6f6a17615d59590600a2d5c6285a384107 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 17 Jun 2020 10:37:36 +0200 Subject: [PATCH] refactor(dev-infra): ng_rollup_bundle rule should leverage `@bazel/rollup` (#37623) Refactors the `ng_rollup_bundle` rule to a macro that relies on the `@bazel/rollup` package. This means that the rule no longer deals with custom ESM5 flavour output, but rather only builds prodmode ES2015 output. This matches the common build output in Angular projects, and optimizations done in CLI where ES2015 is the default optimization input. The motiviation for this change is: * Not duplicating rollup Bazel rules. Instead leveraging the official rollup rule. * Not dealing with a third TS output flavor in Bazel.The ESM5 flavour has the potential of slowing down local development (as it requires compilation replaying) * Updating the rule to be aligned with current CLI optimizations. This also _fixes_ a bug that surfaced in the old rollup bundle rule. Code that is unused, is not removed properly. The new rule fixes this by setting the `toplevel` flag. This instructs terser to remove unused definitions at top-level. This matches the optimization applied in CLI projects. Notably the CLI doesn't need this flag, as code is always wrapped by Webpack. Hence, the unused code eliding runs by default. PR Close #37623 --- dev-infra/bazel/BUILD.bazel | 1 + dev-infra/bazel/expand_template.bzl | 45 ++ .../component_benchmark.bzl | 2 +- .../benchmark/ng_rollup_bundle/BUILD.bazel | 2 +- .../ng_rollup_bundle/ng_rollup_bundle.bzl | 499 +++--------------- .../ng_rollup_bundle/rollup.config-tmpl.js | 89 ++++ .../ng_rollup_bundle/rollup.config.js | 212 -------- .../ng_rollup_bundle/terser_config.json | 24 +- .../transplanted_views/BUILD.bazel | 2 +- .../benchmarks/src/expanding_rows/BUILD.bazel | 2 +- .../src/js-web-frameworks/ng2/BUILD.bazel | 2 +- .../benchmarks/src/largetable/ng2/BUILD.bazel | 2 +- .../benchmarks/src/styling/ng2/BUILD.bazel | 2 +- modules/benchmarks/src/tree/ng2/BUILD.bazel | 2 +- modules/benchmarks/src/views/BUILD.bazel | 4 +- modules/benchmarks/src/views/index.html | 2 +- package.json | 2 +- .../bazel/src/ng_package/rollup.config.js | 4 +- .../example-with-ts-library/package.json | 10 +- .../portal/BUILD.bazel | 2 +- .../example-with-ts-library/utils/BUILD.bazel | 2 +- .../example_with_ts_library_package.golden | 68 +-- packages/core/test/render3/perf/README.md | 10 +- .../core/test/render3/perf/profile_all.js | 5 +- .../test/render3/perf/profile_in_browser.html | 4 +- packages/language-service/bundles/BUILD.bazel | 2 +- packages/language-service/bundles/index.bzl | 25 + packages/service-worker/BUILD.bazel | 2 +- tools/ng_benchmark.bzl | 4 +- yarn.lock | 26 +- 30 files changed, 297 insertions(+), 761 deletions(-) create mode 100644 dev-infra/bazel/BUILD.bazel create mode 100644 dev-infra/bazel/expand_template.bzl create mode 100644 dev-infra/benchmark/ng_rollup_bundle/rollup.config-tmpl.js delete mode 100644 dev-infra/benchmark/ng_rollup_bundle/rollup.config.js create mode 100644 packages/language-service/bundles/index.bzl diff --git a/dev-infra/bazel/BUILD.bazel b/dev-infra/bazel/BUILD.bazel new file mode 100644 index 0000000000..ffd0fb0cdc --- /dev/null +++ b/dev-infra/bazel/BUILD.bazel @@ -0,0 +1 @@ +package(default_visibility = ["//visibility:public"]) diff --git a/dev-infra/bazel/expand_template.bzl b/dev-infra/bazel/expand_template.bzl new file mode 100644 index 0000000000..e21bd9fa1d --- /dev/null +++ b/dev-infra/bazel/expand_template.bzl @@ -0,0 +1,45 @@ +"""Implementation of the expand_template rule """ + +def expand_template_impl(ctx): + substitutions = dict() + + for k in ctx.attr.configuration_env_vars: + if k in ctx.var.keys(): + substitutions["TMPL_%s" % k] = ctx.var[k] + + for k in ctx.attr.substitutions: + substitutions[k] = ctx.expand_location(ctx.attr.substitutions[k], targets = ctx.attr.data) + + ctx.actions.expand_template( + template = ctx.file.template, + output = ctx.outputs.output_name, + substitutions = substitutions, + ) + +"""Rule that can be used to substitute variables in a given template file.""" +expand_template = rule( + implementation = expand_template_impl, + attrs = { + "configuration_env_vars": attr.string_list( + default = [], + doc = "Bazel configuration variables which should be exposed to the template.", + ), + "output_name": attr.output( + mandatory = True, + doc = "File where the substituted template is written to.", + ), + "substitutions": attr.string_dict( + mandatory = True, + doc = "Dictionary of substitutions that should be available to the template. Dictionary key represents the placeholder in the template.", + ), + "data": attr.label_list( + doc = """Data dependencies for location expansion.""", + allow_files = True, + ), + "template": attr.label( + mandatory = True, + allow_single_file = True, + doc = "File used as template.", + ), + }, +) diff --git a/dev-infra/benchmark/component_benchmark/component_benchmark.bzl b/dev-infra/benchmark/component_benchmark/component_benchmark.bzl index 9394cdd69a..edf4d0f4e0 100644 --- a/dev-infra/benchmark/component_benchmark/component_benchmark.bzl +++ b/dev-infra/benchmark/component_benchmark/component_benchmark.bzl @@ -132,7 +132,7 @@ def component_benchmark( bootstrap = ["//packages/zone.js/bundles:zone.umd.js"], port = 4200, static_files = assets + styles, - deps = [":" + app_main + ".min_debug.es2015.js"], + deps = [":" + app_main + ".min_debug.js"], additional_root_paths = ["//dev-infra/benchmark/component_benchmark/defaults"], serving_path = "/app_bundle.js", ) diff --git a/dev-infra/benchmark/ng_rollup_bundle/BUILD.bazel b/dev-infra/benchmark/ng_rollup_bundle/BUILD.bazel index 0b7900ba98..afb8a7a695 100644 --- a/dev-infra/benchmark/ng_rollup_bundle/BUILD.bazel +++ b/dev-infra/benchmark/ng_rollup_bundle/BUILD.bazel @@ -3,7 +3,7 @@ package(default_visibility = ["//visibility:public"]) load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary") exports_files([ - "rollup.config.js", + "rollup.config-tmpl.js", "terser_config.json", ]) diff --git a/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl b/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl index c478ce3df0..a7eb00e66f 100644 --- a/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl +++ b/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl @@ -3,414 +3,90 @@ # 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 -"""Rollup with Build Optimizer - - This provides a variant of the [rollup_bundle] rule that works better for Angular apps. - - It registers `@angular-devkit/build-optimizer` as a rollup plugin, to get - better optimization. It also uses ESM5 format inputs, as this is what - build-optimizer is hard-coded to look for and transform. - - [rollup_bundle]: https://bazelbuild.github.io/rules_nodejs/rollup/rollup_bundle.html -""" - load("@build_bazel_rules_nodejs//:index.bzl", "npm_package_bin") -load("@build_bazel_rules_nodejs//:providers.bzl", "JSEcmaScriptModuleInfo", "NpmPackageInfo", "node_modules_aspect") -load("//packages/bazel/src:esm5.bzl", "esm5_outputs_aspect", "esm5_root_dir", "flatten_esm5") load("@npm_bazel_terser//:index.bzl", "terser_minified") +load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") +load("//dev-infra/bazel:expand_template.bzl", "expand_template") -_NG_ROLLUP_BUNDLE_OUTPUTS = { - "bundle": "%{name}.js", - "sourcemap": "%{name}.js.map", -} +def ng_rollup_bundle( + name, + entry_point, + deps = [], + license_banner = None, + build_optimizer = True, + visibility = None, + format = "iife", + globals = {}, + **kwargs): + """Rollup with Build Optimizer on target prodmode output (ESM2015). -_NG_ROLLUP_MODULE_MAPPINGS_ATTR = "ng_rollup_module_mappings" - -def _ng_rollup_module_mappings_aspect_impl(target, ctx): - mappings = dict() - for dep in ctx.rule.attr.deps: - if hasattr(dep, _NG_ROLLUP_MODULE_MAPPINGS_ATTR): - for k, v in getattr(dep, _NG_ROLLUP_MODULE_MAPPINGS_ATTR).items(): - if k in mappings and mappings[k] != v: - fail(("duplicate module mapping at %s: %s maps to both %s and %s" % - (target.label, k, mappings[k], v)), "deps") - mappings[k] = v - if ((hasattr(ctx.rule.attr, "module_name") and ctx.rule.attr.module_name) or - (hasattr(ctx.rule.attr, "module_root") and ctx.rule.attr.module_root)): - mn = ctx.rule.attr.module_name - if not mn: - mn = target.label.name - mr = target.label.package - if target.label.workspace_root: - mr = "%s/%s" % (target.label.workspace_root, mr) - if ctx.rule.attr.module_root and ctx.rule.attr.module_root != ".": - if ctx.rule.attr.module_root.endswith(".ts"): - # This is the type-checking module mapping. Strip the trailing .d.ts - # as it doesn't belong in TypeScript's path mapping. - mr = "%s/%s" % (mr, ctx.rule.attr.module_root.replace(".d.ts", "")) - else: - mr = "%s/%s" % (mr, ctx.rule.attr.module_root) - if mn in mappings and mappings[mn] != mr: - fail(("duplicate module mapping at %s: %s maps to both %s and %s" % - (target.label, mn, mappings[mn], mr)), "deps") - mappings[mn] = mr - return struct(ng_rollup_module_mappings = mappings) - -ng_rollup_module_mappings_aspect = aspect( - _ng_rollup_module_mappings_aspect_impl, - attr_aspects = ["deps"], -) - -_NG_ROLLUP_BUNDLE_DEPS_ASPECTS = [esm5_outputs_aspect, ng_rollup_module_mappings_aspect, node_modules_aspect] - -_NG_ROLLUP_BUNDLE_ATTRS = { - "build_optimizer": attr.bool( - doc = """Use build optimizer plugin - - Only used if sources are esm5 which depends on value of esm5_sources.""", - default = True, - ), - "esm5_sources": attr.bool( - doc = """Use esm5 input sources""", - default = True, - ), - "srcs": attr.label_list( - doc = """JavaScript source files from the workspace. - These can use ES2015 syntax and ES Modules (import/export)""", - allow_files = True, - ), - "entry_point": attr.label( - doc = """The starting point of the application, passed as the `--input` flag to rollup. - - If the entry JavaScript file belongs to the same package (as the BUILD file), - you can simply reference it by its relative name to the package directory: - - ``` - ng_rollup_bundle( - name = "bundle", - entry_point = ":main.js", - ) - ``` - - You can specify the entry point as a typescript file so long as you also include - the ts_library target in deps: - - ``` - ts_library( - name = "main", - srcs = ["main.ts"], - ) - - ng_rollup_bundle( - name = "bundle", - deps = [":main"] - entry_point = ":main.ts", - ) - ``` - - The rule will use the corresponding `.js` output of the ts_library rule as the entry point. - - If the entry point target is a rule, it should produce a single JavaScript entry file that will be passed to the nodejs_binary rule. - For example: - - ``` - filegroup( - name = "entry_file", - srcs = ["main.js"], - ) - - ng_rollup_bundle( - name = "bundle", - entry_point = ":entry_file", - ) - ``` - """, - mandatory = True, - allow_single_file = True, - ), - "deps": attr.label_list( - doc = """Other targets that provide JavaScript files. - Typically this will be `ts_library` or `ng_module` targets.""", - aspects = _NG_ROLLUP_BUNDLE_DEPS_ASPECTS, - ), - "format": attr.string( - doc = """"Specifies the format of the generated bundle. One of the following: - -- `amd`: Asynchronous Module Definition, used with module loaders like RequireJS -- `cjs`: CommonJS, suitable for Node and other bundlers -- `esm`: Keep the bundle as an ES module file, suitable for other bundlers and inclusion as a `
    -
  1. Build the benchmark using yarn bazel build //packages/core/test/render3/perf:${BENCHMARK}.min_debug.es2015.js --config=ivy
  2. +
  3. Build the benchmark using yarn bazel build //packages/core/test/render3/perf:${BENCHMARK}.min_debug.js --config=ivy
  4. Open this file using the file:// protocol and add ?benchmark=BENCHMARK to the URL.
  5. Note: You should likely run this in an incognito browser with the "no-turbo-inlining" flag.
    diff --git a/packages/language-service/bundles/BUILD.bazel b/packages/language-service/bundles/BUILD.bazel index aec049a2f5..698e333666 100644 --- a/packages/language-service/bundles/BUILD.bazel +++ b/packages/language-service/bundles/BUILD.bazel @@ -1,4 +1,4 @@ -load("//dev-infra/benchmark/ng_rollup_bundle:ng_rollup_bundle.bzl", "ls_rollup_bundle") +load("//packages/language-service/bundles:index.bzl", "ls_rollup_bundle") ls_rollup_bundle( name = "language-service", diff --git a/packages/language-service/bundles/index.bzl b/packages/language-service/bundles/index.bzl new file mode 100644 index 0000000000..ca2498d940 --- /dev/null +++ b/packages/language-service/bundles/index.bzl @@ -0,0 +1,25 @@ +load("//dev-infra/benchmark/ng_rollup_bundle:ng_rollup_bundle.bzl", "ng_rollup_bundle") + +def ls_rollup_bundle(name, **kwargs): + """ + A variant of ng_rollup_bundle for the language-service bundle that + outputs in AMD format. + """ + visibility = kwargs.pop("visibility", None) + + # Note: the output file is called "umd.js" because of historical reasons. + # The format is actually AMD and not UMD, but we are afraid to rename + # the file because that would likely break the IDE and other integrations that + # have the path hardcoded in them. + ng_rollup_bundle( + name = name + ".umd", + build_optimizer = False, + format = "amd", + visibility = visibility, + **kwargs + ) + native.alias( + name = name, + actual = name + ".umd", + visibility = visibility, + ) diff --git a/packages/service-worker/BUILD.bazel b/packages/service-worker/BUILD.bazel index 5f5c60abeb..0d3ea6c507 100644 --- a/packages/service-worker/BUILD.bazel +++ b/packages/service-worker/BUILD.bazel @@ -26,7 +26,7 @@ genrule( genrule( name = "ngsw_worker_renamed", - srcs = ["//packages/service-worker/worker:ngsw_worker.es2015.js"], + srcs = ["//packages/service-worker/worker:ngsw_worker.js"], outs = ["ngsw-worker.js"], # Remove sourcemap since this file will be served in production site # See https://github.com/angular/angular/issues/23596 diff --git a/tools/ng_benchmark.bzl b/tools/ng_benchmark.bzl index 3df440e80a..d9239c2aee 100644 --- a/tools/ng_benchmark.bzl +++ b/tools/ng_benchmark.bzl @@ -19,14 +19,14 @@ def ng_benchmark(name, bundle): nodejs_binary( name = name, data = [bundle], - entry_point = bundle + ".min_debug.es2015.js", + entry_point = bundle + ".min_debug.js", tags = ["local", "manual"], # run benchmarks locally and never on CI ) nodejs_binary( name = name + "_profile", data = [bundle], - entry_point = bundle + ".min_debug.es2015.js", + entry_point = bundle + ".min_debug.js", args = ["--node_options=--no-turbo-inlining --node_options=--inspect-brk"], tags = ["local", "manual"], # run benchmarks locally and never on CI ) diff --git a/yarn.lock b/yarn.lock index 3c26cfb40b..9155224971 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2160,11 +2160,6 @@ dependencies: "@types/node" "*" -"@types/estree@*": - version "0.0.44" - resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.44.tgz#980cc5a29a3ef3bea6ff1f7d021047d7ea575e21" - integrity sha512-iaIVzr+w2ZJ5HkidlZ3EJM8VTZb2MJLCjw3V+505yVts0gRC4UMvjw0d1HPtGqI/HQC/KdsYtayfzl+AXY2R8g== - "@types/estree@0.0.39": version "0.0.39" resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f" @@ -2577,11 +2572,6 @@ acorn@^6.1.1, acorn@^6.4.1: resolved "https://registry.yarnpkg.com/acorn/-/acorn-6.4.1.tgz#531e58ba3f51b9dacb9a6646ca4debf5b14ca474" integrity sha512-ZVA9k326Nwrj3Cj9jlh3wGFutC2ZornPNARZwsNYqQYgN0EsV2d53w5RN/co65Ohn4sUAUtb1rSUAOD6XN9idA== -acorn@^7.1.0: - version "7.1.1" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-7.1.1.tgz#e35668de0b402f359de515c5482a1ab9f89a69bf" - integrity sha512-add7dgA5ppRPxCFJoAGfMDi7PIBXq1RtGo7BhbLaxwrXPOmw8gq48Y9ozT01hUKy9byMjlR20EJhu5zlkErEkg== - add-stream@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/add-stream/-/add-stream-1.0.0.tgz#6a7990437ca736d5e1288db92bd3266d5f5cb2aa" @@ -13310,6 +13300,13 @@ rollup@2.10.9: optionalDependencies: fsevents "~2.1.2" +rollup@^2.16.1: + version "2.16.1" + resolved "https://registry.yarnpkg.com/rollup/-/rollup-2.16.1.tgz#97805e88071e2c6727bd0b64904976d14495c873" + integrity sha512-UYupMcbFtoWLB6ZtL4hPZNUTlkXjJfGT33Mmhz3hYLNmRj/cOvX2B26ZxDQuEpwtLdcyyyraBGQ7EfzmMJnXXg== + optionalDependencies: + fsevents "~2.1.2" + rollup@~1.11.3: version "1.11.3" resolved "https://registry.yarnpkg.com/rollup/-/rollup-1.11.3.tgz#6f436db2a2d6b63f808bf60ad01a177643dedb81" @@ -13319,15 +13316,6 @@ rollup@~1.11.3: "@types/node" "^11.13.9" acorn "^6.1.1" -rollup@~1.25.0: - version "1.25.2" - resolved "https://registry.yarnpkg.com/rollup/-/rollup-1.25.2.tgz#739f508bd8f7ece52bb6c1fcda83466af82b7f6d" - integrity sha512-+7z6Wab/L45QCPcfpuTZKwKiB0tynj05s/+s2U3F2Bi7rOLPr9UcjUwO7/xpjlPNXA/hwnth6jBExFRGyf3tMg== - dependencies: - "@types/estree" "*" - "@types/node" "*" - acorn "^7.1.0" - router@^1.3.1: version "1.3.5" resolved "https://registry.yarnpkg.com/router/-/router-1.3.5.tgz#cb2f47f74fd99a77fb3bc01cc947f46b79b1790f"