From 46ef4cdc3425fd49d813a99a968097044061502f Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 11 Sep 2019 19:23:21 -0700 Subject: [PATCH] refactor: simplify API a bit Based on discussion with @fenghaolw --- e2e/webapp/BUILD.bazel | 4 +- packages/rollup/src/rollup_bundle.bzl | 9 +-- .../rollup/test/code_splitting/BUILD.bazel | 2 +- packages/rollup/test/code_splitting/spec.js | 4 +- .../test/multiple_entry_points/BUILD.bazel | 6 +- packages/terser/src/terser_minified.bzl | 55 ++++++------------- packages/terser/test/debug/BUILD.bazel | 5 +- .../terser/test/directory_input/BUILD.bazel | 2 +- packages/terser/test/user_config/BUILD.bazel | 3 +- 9 files changed, 35 insertions(+), 55 deletions(-) diff --git a/e2e/webapp/BUILD.bazel b/e2e/webapp/BUILD.bazel index 2b98623f99..6ad481e124 100644 --- a/e2e/webapp/BUILD.bazel +++ b/e2e/webapp/BUILD.bazel @@ -6,12 +6,12 @@ rollup_bundle( name = "app", srcs = ["strings.js"], entry_point = "app.js", - output_dir = "bundle", + output_dir = True, ) terser_minified( name = "out.min", - src_dir = "app", + src = "app", ) nodejs_test( diff --git a/packages/rollup/src/rollup_bundle.bzl b/packages/rollup/src/rollup_bundle.bzl index 45ce1bd94c..feb89697c0 100644 --- a/packages/rollup/src/rollup_bundle.bzl +++ b/packages/rollup/src/rollup_bundle.bzl @@ -132,10 +132,11 @@ Passed to the [`--globals` option](https://github.com/rollup/rollup/blob/master/ Also, the keys from the map are passed to the [`--external` option](https://github.com/rollup/rollup/blob/master/docs/999-big-list-of-options.md#external). """, ), - "output_dir": attr.string( - doc = """A directory in which generated chunks are placed. + "output_dir": attr.bool( + doc = """Whether to produce a directory output. -Passed to the [`--output.dir` option](https://github.com/rollup/rollup/blob/master/docs/999-big-list-of-options.md#outputdir) in rollup. +We will use the [`--output.dir` option](https://github.com/rollup/rollup/blob/master/docs/999-big-list-of-options.md#outputdir) in rollup +rather than `--output.file`. If the program produces multiple chunks, you must specify this attribute. Otherwise, the outputs are assumed to be a single file. @@ -240,7 +241,7 @@ def _rollup_bundle(ctx): # If user requests an output_dir, then use output.dir rather than output.file if ctx.attr.output_dir: - outputs.append(ctx.actions.declare_directory(ctx.attr.output_dir)) + outputs.append(ctx.actions.declare_directory(ctx.label.name)) for entry_point in entry_points: args.add_joined([entry_point[1], _no_ext(entry_point[0])], join_with = "=") args.add_all(["--output.dir", outputs[0].path]) diff --git a/packages/rollup/test/code_splitting/BUILD.bazel b/packages/rollup/test/code_splitting/BUILD.bazel index 9bac6eff23..256f3cc22d 100644 --- a/packages/rollup/test/code_splitting/BUILD.bazel +++ b/packages/rollup/test/code_splitting/BUILD.bazel @@ -9,7 +9,7 @@ rollup_bundle( # TODO: if this isn't here, the error is # [!] Error: You must set output.dir instead of output.file when generating multiple chunks. # which will confuse users because it references output.dir instead of output_dir - output_dir = "chunks", + output_dir = True, ) jasmine_node_test( diff --git a/packages/rollup/test/code_splitting/spec.js b/packages/rollup/test/code_splitting/spec.js index 0d81686903..dbea836849 100644 --- a/packages/rollup/test/code_splitting/spec.js +++ b/packages/rollup/test/code_splitting/spec.js @@ -2,7 +2,7 @@ const fs = require('fs'); describe('rollup code splitting', () => { it('should produce a chunk for lazy loaded code', () => { - expect(fs.existsSync(require.resolve(__dirname + '/chunks/bundle.js'))).toBeTruthy(); - expect(fs.existsSync(require.resolve(__dirname + '/chunks/chunk.js'))).toBeTruthy(); + expect(fs.existsSync(require.resolve(__dirname + '/bundle/bundle.js'))).toBeTruthy(); + expect(fs.existsSync(require.resolve(__dirname + '/bundle/chunk.js'))).toBeTruthy(); }); }); diff --git a/packages/rollup/test/multiple_entry_points/BUILD.bazel b/packages/rollup/test/multiple_entry_points/BUILD.bazel index 356abc88bf..72f97b74ca 100644 --- a/packages/rollup/test/multiple_entry_points/BUILD.bazel +++ b/packages/rollup/test/multiple_entry_points/BUILD.bazel @@ -2,17 +2,17 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test") load("@npm_bazel_rollup//:index.from_src.bzl", "rollup_bundle") rollup_bundle( - name = "bundle", + name = "chunks", entry_points = { "entry1.js": "one", "entry2.js": "two", }, - output_dir = "chunks", + output_dir = True, ) jasmine_node_test( name = "test", srcs = ["spec.js"], data = ["@npm//source-map"], - deps = [":bundle"], + deps = [":chunks"], ) diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl index 599546d7ff..a0fffec9d7 100644 --- a/packages/terser/src/terser_minified.bzl +++ b/packages/terser/src/terser_minified.bzl @@ -29,15 +29,19 @@ terser_minified( Note that the `name` attribute determines what the resulting files will be called. So the example above will output `out.min.js` and `out.min.js.map` (since `sourcemap` defaults to `true`). +If the input is a directory, then the output will also be a directory, named after the `name` attribute. """ _TERSER_ATTRS = { "src": attr.label( - doc = """A JS file, or a rule producing .js files as its default output + doc = """File(s) to minify. + +Can be a .js file, a rule producing .js files as its default output, or a rule producing a directory of .js files. Note that you can pass multiple files to terser, which it will bundle together. If you want to do this, you can pass a filegroup here.""", allow_files = [".js"], + mandatory = True, ), "config_file": attr.label( doc = """A JSON file containing Terser minify() options. @@ -83,14 +87,6 @@ so that it only affects the current build. doc = "Whether to produce a .js.map output", default = True, ), - "src_dir": attr.label( - doc = """A directory containing some .js files. - -Each `.js` file will be run through terser, and the rule will output a directory of minified files. -The output will be a directory named the same as the "name" attribute. -Any files not ending in `.js` will be ignored. -""", - ), "terser_bin": attr.label( doc = "An executable target that runs Terser", default = Label("@npm//@bazel/terser/bin:terser"), @@ -99,41 +95,25 @@ Any files not ending in `.js` will be ignored. ), } -def _terser_outs(src_dir, sourcemap): - if src_dir: - # Tree artifact outputs must be declared with ctx.actions.declare_directory - return {} - result = {"minified": "%{name}.js"} - if sourcemap: - result["sourcemap"] = "%{name}.js.map" - return result - def _terser(ctx): "Generate actions to create terser config run terser" # CLI arguments; see https://www.npmjs.com/package/terser#command-line-usage args = ctx.actions.args() - inputs = [] - outputs = [getattr(ctx.outputs, o) for o in dir(ctx.outputs)] - - if ctx.attr.src and ctx.attr.src_dir: - fail("Only one of src and src_dir attributes should be specified") - if not ctx.attr.src and not ctx.attr.src_dir: - fail("Either src or src_dir is required") - if ctx.attr.src: - for src in ctx.files.src: - if src.is_directory: - fail("Directories should be specified in the src_dir attribute, not src") - args.add(src.path) - inputs.extend(ctx.files.src) - else: - for src in ctx.files.src_dir: - if not src.is_directory: - fail("Individual files should be specifed in the src attribute, not src_dir") - args.add(src.path) - inputs.extend(ctx.files.src_dir) + inputs = ctx.files.src[:] + outputs = [] + + directory_srcs = [s for s in ctx.files.src if s.is_directory] + if len(directory_srcs) > 0: + if len(ctx.files.src) > 1: + fail("When directories are passed to terser_minified, there should be only one input") outputs.append(ctx.actions.declare_directory(ctx.label.name)) + else: + outputs.append(ctx.actions.declare_file("%s.js" % ctx.label.name)) + if ctx.attr.sourcemap: + outputs.append(ctx.actions.declare_file("%s.js.map" % ctx.label.name)) + args.add_all([s.path for s in ctx.files.src]) args.add_all(["--output", outputs[0].path]) debug = ctx.attr.debug or "DEBUG" in ctx.var.keys() @@ -182,5 +162,4 @@ terser_minified = rule( doc = _DOC, implementation = _terser, attrs = _TERSER_ATTRS, - outputs = _terser_outs, ) diff --git a/packages/terser/test/debug/BUILD.bazel b/packages/terser/test/debug/BUILD.bazel index d5c1cd5f34..ca2574fead 100644 --- a/packages/terser/test/debug/BUILD.bazel +++ b/packages/terser/test/debug/BUILD.bazel @@ -11,8 +11,6 @@ terser_minified( golden_file_test( name = "test", - # Assert that the default output is a .js file - # by omitting the ".js" extension here actual = "out.min", golden = "output.golden.js_", ) @@ -20,13 +18,14 @@ golden_file_test( terser_minified( name = "debug_from_env", src = "input.js", + sourcemap = False, # Don't specify debug = True # Instead we'll run the test with --define=DEBUG=1 ) golden_file_test( name = "test_define_DEBUG", - actual = "debug_from_env.js", + actual = "debug_from_env", golden = "output.golden.js_", tags = ["manual"], # Only passes when --define=DEBUG=1 is set ) diff --git a/packages/terser/test/directory_input/BUILD.bazel b/packages/terser/test/directory_input/BUILD.bazel index a08f15e84f..2bfffadde5 100644 --- a/packages/terser/test/directory_input/BUILD.bazel +++ b/packages/terser/test/directory_input/BUILD.bazel @@ -10,9 +10,9 @@ declare_directory( terser_minified( name = "out.min", + src = "dir", # TODO: support sourcemaps too sourcemap = False, - src_dir = "dir", ) jasmine_node_test( diff --git a/packages/terser/test/user_config/BUILD.bazel b/packages/terser/test/user_config/BUILD.bazel index 944b5be545..51fc77dc6d 100644 --- a/packages/terser/test/user_config/BUILD.bazel +++ b/packages/terser/test/user_config/BUILD.bazel @@ -5,11 +5,12 @@ terser_minified( name = "out.min", src = "input.js", config_file = "terser_config.json", + sourcemap = False, ) golden_file_test( name = "test", - actual = "out.min.js", + actual = "out.min", golden = "output.golden.js_", golden_debug = "output.debug.golden.js_", )