feat(rollup): provide sourcemaps for all output types #641

jbedard · 2019-03-31T22:58:17Z

~~This adds sourcemap targets output groups for all the output formats.~~

Today there's the default target which outputs the es5 bundle.js and bundle.js.map. Then there's sub-targets such as .es2015.js, .umd.js etc. There's also .js which is the same as the default target but without the .map. But there's no way to easily fetch the .js+map of only the es2015 bundle.

~~This adds a .js.map target alongside every .js target.~~
This adds an output-group for each bundle type. The tests show an example of filegroups consuming these output-groups.

jbedard · 2019-03-31T22:59:19Z

internal/rollup/rollup_bundle.bzl

-            template = ctx.file._no_explore_html,
-            substitutions = {},
-        )
+        # There is no source map or explorer output when code-splitting but we still need to satisfy the output


Seems like there's no source maps when code-splitting today? I guess that could be a future task...

what does the rollup program produce? I bet it still creates sourcemaps, and having a size explorer for chunks is a pretty critical feature, could fix this next

jbedard · 2019-03-31T23:01:32Z

internal/rollup/rollup_bundle.bzl

-        run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min_debug, debug = True)
+        source_map_min = run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min)
+        run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min_debug, debug = True, in_source_map = source_map_min)
+


I guess we could just copy the source_map_min to ctx.outputs.build_es5_map just so the build_es5_map target exists for consistency?

But it would have the wrong content, wouldn't it? I think we could just fix #175 next

jbedard · 2019-03-31T23:01:48Z

internal/rollup/rollup_bundle.bzl

-        run_sourcemapexplorer(ctx, ctx.outputs.build_es5_min, source_map, ctx.outputs.explore_html)
-        files = [ctx.outputs.build_es5_min, source_map]
+
+        # TODO: ?


Is this worth doing for every sourcemap?

The user needs to be able to reason about what makes their bundle large under different cases. If two flavors of JS output could have different size characteristics, then the user would have to be able to debug them separately. But we shouldn't expose more of these sourcemapexplorer things than necessary.

Also I feel like we have gone too far with including lots of behaviors in this rule. Maybe sourcemapexplorer should be a separate rule, WDYT?

alexeagle · 2019-04-02T15:07:01Z

internal/e2e/rollup/bundle-cjs_golden.js.map

@@ -0,0 +1 @@
+{"version":3,"file":"bundle.cjs.js.map","sources":["bundle.es6/internal/e2e/rollup/fum/index.js","../../../../../../../../../../../execroot/build_bazel_rules_nodejs/bazel-out/__platform__-fastbuild/bin/internal/e2e/rollup/bundle.es6/external/npm/node_modules/hello/index.js","bundle.es6/internal/e2e/rollup/bar.js","bundle.es6/internal/e2e/rollup/foo.js"],"sourcesContent":["export const fum = 'Wonderland';\n","export default 'Hello';\n","export const name = 'Alice';\n","import {fum} from 'fumlib';\nimport hello from 'hello';\nimport {thing} from 'some_global_var';\n\nimport {name} from './bar';\n\nconsole.log(`${hello}, ${name} in ${fum}`);\n\n// Tests for @PURE annotations\n/*@__PURE__*/\nconsole.log('side-effect');\n\nclass Impure {\n  constructor() {\n    console.log('side-effect')\n  }\n}\n\n/*@__PURE__*/new Impure();\n\n// Test for sequences = false\nexport class A {\n  a() {\n    return document.a;\n  }\n}\nfunction inline_me() {\n  return 'abc';\n}\nconsole.error(new A().a(), inline_me(), thing);\n"],"names":["thing"],"mappings":";;;;;;;;;;;AAAO,MAAM,GAAG,GAAG,YAAY,CAAC;;ACAhC,YAAe,OAAO,CAAC;;ACAhB,MAAM,IAAI,GAAG,OAAO,CAAC;;ACM5B,OAAO,CAAC,GAAG,CAAC,CAAC,EAAE,KAAK,CAAC,EAAE,EAAE,IAAI,CAAC,IAAI,EAAE,GAAG,CAAC,CAAC,CAAC,CAAC;AAC3C,AAYA;;AAEA,AAAO,MAAM,CAAC,CAAC;EACb,CAAC,GAAG;IACF,OAAO,QAAQ,CAAC,CAAC,CAAC;GACnB;CACF;AACD,SAAS,SAAS,GAAG;EACnB,OAAO,KAAK,CAAC;CACd;AACD,OAAO,CAAC,KAAK,CAAC,IAAI,CAAC,EAAE,CAAC,CAAC,EAAE,EAAE,SAAS,EAAE,EAAEA,qBAAK,CAAC,CAAC;;;;"}


I'm questioning the volume of golden content here. Do we really want to assert on the content of all the sourcemaps? If you just want to assert that the rollup_bundle rule has some output file, we can do that just by referencing it as an input to any rule (like a trivial genrule)

alexeagle · 2019-04-02T15:08:51Z

internal/rollup/rollup_bundle.bzl

-        run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min_debug, debug = True)
+        source_map_min = run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min)
+        run_terser(ctx, ctx.outputs.build_es5, ctx.outputs.build_es5_min_debug, debug = True, in_source_map = source_map_min)
+


But it would have the wrong content, wouldn't it? I think we could just fix #175 next

alexeagle · 2019-04-02T15:11:06Z

internal/rollup/rollup_bundle.bzl

-        run_sourcemapexplorer(ctx, ctx.outputs.build_es5_min, source_map, ctx.outputs.explore_html)
-        files = [ctx.outputs.build_es5_min, source_map]
+
+        # TODO: ?


The user needs to be able to reason about what makes their bundle large under different cases. If two flavors of JS output could have different size characteristics, then the user would have to be able to debug them separately. But we shouldn't expose more of these sourcemapexplorer things than necessary.

Also I feel like we have gone too far with including lots of behaviors in this rule. Maybe sourcemapexplorer should be a separate rule, WDYT?

alexeagle · 2019-04-02T15:11:39Z

internal/rollup/rollup_bundle.bzl

@@ -664,13 +690,22 @@ ROLLUP_ATTRS = {

 ROLLUP_OUTPUTS = {
    "build_cjs": "%{name}.cjs.js",
+    "build_cjs_map": "%{name}.cjs.js.map",


this needs some re-organization as a starlark provider so the JS and .map can stay together as tuple through the dependent build steps...

This seems like the ideal use case for OutputGroupInfo provider
https://docs.bazel.build/versions/master/skylark/rules.html#requesting-output-files
https://docs.bazel.build/versions/master/skylark/lib/OutputGroupInfo.html

👍

Would we still want the targets for individual files? Only the .js ones which exist today?

I got this working locally, but does this mean people must use the --output_groups flag instead of something similar to the bundle.es2015.js used today?

Note that I dropped the ROLLUP_OUTPUTS and only used declare_file, DefaultInfo and OutputGroupInfo

alexeagle

I think OutputGroup may be the wrong thing - it seems that it makes it impossible for a downstream rule to request a particular output? Like https://github.com/angular/angular-bazel-example/blob/master/src/BUILD.bazel#L179

alexeagle · 2019-04-08T23:18:55Z

internal/e2e/check.js

-      .replace(/\\r\\n/g, '\\n');
+      .replace(/\\r\\n/g, '\\n')
+      // Replace platform specific directories in paths
+      .replace(/\/\w+-fastbuild\//g, '/__platform__-fastbuild/')


is this related? could be a separate PR?

I was getting different folder names outputted here compared to circleci. Were the golden files previously generated by circleci so they have the circleci platform? I figured this was better then me manually editing the golden files...

kyliau · 2019-04-08T23:58:52Z

I think OutputGroup may be the wrong thing - it seems that it makes it impossible for a downstream rule to request a particular output? Like https://github.com/angular/angular-bazel-example/blob/master/src/BUILD.bazel#L179

If ROLLUP_OUTPUTS is restored then downstream targets could continue to request a specific output as before.

I think OutputGroup could still be useful, because it provides logical grouping of the outputs.
The downside is that downstream targets would need to declare an intermediate filegroup in order to retrieve the sourcemap.

rollup_bundle(
    name = "bundle",
    entry_point = "src/main.prod",
    deps = [
        // ...
    ],
)

filegroup(
    name = "bundle_es2015_min_with_sourcemap"
    srcs = [":bundle"],
    output_group = "es2015_min",
)

web_package(
    name = "prodapp",
    assets = _ASSETS + [
        ":bundle.min.js",
        ":bundle.min.es2015.js",
    ],
    data = [
        ":bundle_es2015_min_with_sourcemap",
    ],
    index_html = "index.html",
)

jbedard · 2019-04-09T19:46:23Z

I've added a commit changing rollup_bundle to a macro which runs rollup as well as declaring a bunch of filegroup targets for a) each bundle type .js and .map b) just the .js file.

I think this is working, but I'm not sure if the macro+filegroups is the best solution, and haven't fully tested it. @alexeagle I'm not sure what you meant by "starlark provider", is that another way of doing this?

jbedard · 2019-04-13T02:06:30Z

Update to just add an OutputGroupInfo to the result, with one group per bundle type. Without specifically consuming these new output-groups no one should notice this change.

Since this PR is now just this small output-group addition I've greatly simplified the diff and removed anything unnecessary.

Instead of adding new tests I've just executed the existing tests (a second time), but using the output-group instead of default info.

kyliau · 2019-04-13T22:13:34Z

internal/e2e/rollup/BUILD.bazel

+
+jasmine_node_test(
+    name = "test-outputgroups",
+    srcs = glob(["*.spec.js"]),


This should probably be another spec file that asserts the existence of sourcemap for each flavor in addition to the js file. Existing rollup.spec.js only checks sourcemap for the default output.

jbedard · 2019-04-14T06:08:18Z

I've changed the tests to assert the presence of the .map files alongside .js when using the output groups.

kyliau

LGTM. Could you please take a look at the test failure on CI? The test failed on Windows, so probably it's just a path issue in the spec.

kyliau · 2019-04-15T22:03:21Z

Output groups could be requested on the command line.
For example,

bazel build :bundle --output_groups=es2015_min

See https://docs.bazel.build/versions/master/command-line-reference.html#flag--output_groups

jbedard requested review from alexeagle, gregmagolan and kyliau as code owners March 31, 2019 22:58

googlebot added the cla: yes label Mar 31, 2019

jbedard commented Mar 31, 2019

View reviewed changes

jbedard force-pushed the source-maps branch 2 times, most recently from 1e9086a to e27f62f Compare April 1, 2019 05:08

alexeagle reviewed Apr 2, 2019

View reviewed changes

alexeagle reviewed Apr 8, 2019

View reviewed changes

jbedard force-pushed the source-maps branch 6 times, most recently from 3c0819f to b1cf64c Compare April 13, 2019 01:59

jbedard force-pushed the source-maps branch from b1cf64c to 98968de Compare April 13, 2019 02:08

kyliau reviewed Apr 13, 2019

View reviewed changes

jbedard force-pushed the source-maps branch from fd3b80b to 5031d30 Compare April 14, 2019 06:07

kyliau approved these changes Apr 15, 2019

View reviewed changes

feat(rollup): provide output groupings of js+map for each bundle type

547c2e6

jbedard force-pushed the source-maps branch from 6e73a8e to 547c2e6 Compare April 15, 2019 21:47

alexeagle approved these changes Apr 15, 2019

View reviewed changes

kyliau merged commit 354c25a into bazel-contrib:master Apr 15, 2019

		@@ -0,0 +1 @@
		{"version":3,"file":"bundle.cjs.js.map","sources":["bundle.es6/internal/e2e/rollup/fum/index.js","../../../../../../../../../../../execroot/build_bazel_rules_nodejs/bazel-out/__platform__-fastbuild/bin/internal/e2e/rollup/bundle.es6/external/npm/node_modules/hello/index.js","bundle.es6/internal/e2e/rollup/bar.js","bundle.es6/internal/e2e/rollup/foo.js"],"sourcesContent":["export const fum = 'Wonderland';\n","export default 'Hello';\n","export const name = 'Alice';\n","import {fum} from 'fumlib';\nimport hello from 'hello';\nimport {thing} from 'some_global_var';\n\nimport {name} from './bar';\n\nconsole.log(`${hello}, ${name} in ${fum}`);\n\n// Tests for @PURE annotations\n/@__PURE__/\nconsole.log('side-effect');\n\nclass Impure {\n constructor() {\n console.log('side-effect')\n }\n}\n\n/@__PURE__/new Impure();\n\n// Test for sequences = false\nexport class A {\n a() {\n return document.a;\n }\n}\nfunction inline_me() {\n return 'abc';\n}\nconsole.error(new A().a(), inline_me(), thing);\n"],"names":["thing"],"mappings":";;;;;;;;;;;AAAO,MAAM,GAAG,GAAG,YAAY,CAAC;;ACAhC,YAAe,OAAO,CAAC;;ACAhB,MAAM,IAAI,GAAG,OAAO,CAAC;;ACM5B,OAAO,CAAC,GAAG,CAAC,CAAC,EAAE,KAAK,CAAC,EAAE,EAAE,IAAI,CAAC,IAAI,EAAE,GAAG,CAAC,CAAC,CAAC,CAAC;AAC3C,AAYA;;AAEA,AAAO,MAAM,CAAC,CAAC;EACb,CAAC,GAAG;IACF,OAAO,QAAQ,CAAC,CAAC,CAAC;GACnB;CACF;AACD,SAAS,SAAS,GAAG;EACnB,OAAO,KAAK,CAAC;CACd;AACD,OAAO,CAAC,KAAK,CAAC,IAAI,CAAC,EAAE,CAAC,CAAC,EAAE,EAAE,SAAS,EAAE,EAAEA,qBAAK,CAAC,CAAC;;;;"}

feat(rollup): provide sourcemaps for all output types #641

feat(rollup): provide sourcemaps for all output types #641

Uh oh!

Conversation

jbedard commented Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbedard Apr 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyliau commented Apr 8, 2019

Uh oh!

jbedard commented Apr 9, 2019

Uh oh!

jbedard commented Apr 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbedard commented Apr 14, 2019

Uh oh!

kyliau left a comment

Choose a reason for hiding this comment

Uh oh!

kyliau commented Apr 15, 2019

Uh oh!

Uh oh!

jbedard commented Mar 31, 2019 •

edited

Loading

jbedard Apr 7, 2019 •

edited

Loading