build(ivy): create hello world rollup #22004

alexeagle · 2018-02-02T23:27:34Z

This is a customization of the rollup_bundle rule from rules_nodejs
which adds the build-optimizer as a plugin.

Add a functional test with fast round-trip that asserts the minified app
still works.

Publish the min.js artifact on circleCI so we can track its size.

mary-poppins · 2018-02-02T23:35:21Z

You can preview dfcd36d at https://pr22004-dfcd36d.ngbuilds.io/.

mary-poppins · 2018-02-03T00:28:16Z

You can preview 16af609 at https://pr22004-16af609.ngbuilds.io/.

mary-poppins · 2018-02-03T00:35:18Z

You can preview 0352989 at https://pr22004-0352989.ngbuilds.io/.

mary-poppins · 2018-02-03T00:47:00Z

You can preview 4ac0f20 at https://pr22004-4ac0f20.ngbuilds.io/.

IgorMinar · 2018-02-03T00:24:30Z

WORKSPACE

-    remote = "https://github.com/bazelbuild/rules_nodejs.git",
-    commit = "230d39a391226f51c03448f91eb61370e2e58c42",
+    remote = "https://github.com/alexeagle/rules_nodejs.git",
+    commit = "6a4b9aa32677cbab3dfed48b9b4eda13d8c41f23",


please describe why we use custom build and what needs to be done to get back to upstream. Is there an issue/pr tracking the work?

Landed the changes, so this now points back to HEAD on the upstream repo master

IgorMinar · 2018-02-03T00:25:05Z

WORKSPACE

+git_repository(
+    name = "angular_devkit",
+    remote = "https://github.com/alexeagle/devkit.git",
+    commit = "4b960f37f49a4ed87d54402ce935a4fa0da7b0f6",


do we need this particular commit? can't we use a tag instead? it's much easier to understand what we use if we use tags.

yeah this has a pending PR too

Landed the changes, so this now points back to HEAD on the upstream repo master

IgorMinar · 2018-02-03T00:26:53Z

packages/bazel/src/BUILD.bazel

+nodejs_binary(
+    name = "rollup_with_build_optimizer",
+    data = ["@angular_devkit//packages/angular_devkit/build_optimizer:lib"],
+    entry_point = "build_bazel_rules_nodejs_rollup_deps/node_modules/rollup/bin/rollup",


ouch.. really? why do we reach into that external rule? that looks super ugly. can't we do better?

We could have our own external repo to hold the runtime deps, but since this rule extends rollup_bundle I think it's appropriate to share the runtime deps too

can you document this reasoning. I'm sure more people will wonder about this in the future when looking at this code.

IgorMinar · 2018-02-03T00:35:48Z

packages/core/test/bundling/hello_world/BUILD.bazel

+
+ng_rollup_bundle(
+    name = "bundle",
+    # FIXME(alex): should start with "angular/"


can you capture why it can't be that way now?

It's pretty complicated, basically I want users to see a consistent view that looks like a runfiles view where paths start with the workspace, but in the execroot they don't currently. I need to find the right place to enforce a mapping.

see.. it's not that complicated after all. your description above is way better than the current fixme note.

IgorMinar · 2018-02-03T00:42:06Z

packages/bazel/src/es5_esm.bzl

+"""
+
+# The provider downstream rules use to access the outputs
+ES5ESMInfo = provider(


the terminology, we usually use for this is ESM5 not ES5ESM. see https://docs.google.com/document/d/1CZC2rcpxffTDfRDs6p1cfbmKNLA6x5O-NtkJglDaBVs/preview

IgorMinar · 2018-02-03T00:44:13Z

packages/bazel/src/es5_esm.bzl

+    return []
+
+  # We create a new tsconfig.json file that will have our compilation settings
+  tsconfig = ctx.actions.declare_file("%s_es5_esm.tsconfig.json" % target.label.name)


esm5.tsconfig.json

IgorMinar · 2018-02-03T00:44:36Z

packages/bazel/src/es5_esm.bzl

+  workspace = target.label.workspace_root if target.label.workspace_root else ""
+
+  # re-root the outputs under a ".es5_esm" directory so the path don't collide
+  out_dir = ctx.label.name + ".es5_esm"


esm5... throughout the PR

IgorMinar · 2018-02-03T00:46:39Z

packages/bazel/src/modify_tsconfig.js

+        data['angularCompilerOptions']['expectedOut'].map(
+            f => f.replace(/\.closure\.js$/, '.js').replace(binDir, path.join(binDir, newRoot)));
+  }
+  fs.writeFileSync(output, JSON.stringify(data), {encoding: 'utf-8'});


utf8 is the default for writeFileSync

IgorMinar · 2018-02-03T00:50:13Z

packages/bazel/src/ng_rollup_bundle.bzl

+  # We don't expect anyone to make use of this ES6 bundle, but it makes this rule
+  # compatible with rollup_bundle which allows them to be easily swapped back and
+  # forth.
+  es6_rollup_config = write_rollup_config(ctx, filename = "_%s.rollup_es6.conf.js")


es2015 - can we please be consistent. otherwise it's confusing. esX is no longer a thing. ES2018 has just been finalized: http://2ality.com/2017/02/ecmascript-2018.html

well, this is in a lot of places like rules_typescript, so we can't be 100% consistent without a lot of work, but will fix here

IgorMinar · 2018-02-03T00:53:52Z

.circleci/config.yml

@@ -64,6 +64,10 @@ jobs:
      # See https://github.com/bazelbuild/bazel/issues/4257
      - run: bazel query --output=label '//modules/... union //packages/... union //tools/...' | xargs bazel test --config=ci

+      - store_artifacts:


can you document why we do this? via an inline comment.

it's not clear to me how will the size tracking work? using an external service?

mary-poppins · 2018-02-03T01:23:49Z

You can preview 85f77db at https://pr22004-85f77db.ngbuilds.io/.

mary-poppins · 2018-02-03T01:47:21Z

You can preview 5110990 at https://pr22004-5110990.ngbuilds.io/.

IgorMinar · 2018-02-03T16:27:33Z

packages/bazel/src/ng_rollup_bundle.bzl

+"""This provides a variant of rollup_bundle that works better for Angular apps.
+
+   It registers @angular-devkit/build-optimizer as a rollup plugin, to get
+   better optimization. It also uses ES5-ESM format inputs, as this is what


IgorMinar · 2018-02-03T16:32:05Z

packages/bazel/src/ng_rollup_bundle.bzl

+    "write_rollup_config",
+    "run_rollup",
+    "run_uglify")
+load("@build_bazel_rules_nodejs//internal:collect_es6_sources.bzl", collect_es2015_sources = "collect_es6_sources")


I filed bazelbuild/rules_nodejs#107

IgorMinar · 2018-02-03T16:32:28Z

packages/bazel/src/ng_rollup_bundle.bzl

+BO_PLUGIN="require('angular_devkit/packages/angular_devkit/build_optimizer/src/build-optimizer/rollup-plugin.js').default()"
+
+def _ng_rollup_bundle(ctx):
+  # We don't expect anyone to make use of this ES6 bundle, but it makes this rule


it's esm2015 bundle or fesm2015

also we do very much expect to start switching over to esm2015 as the primary input to application bundle around the v7 timeframe.

IgorMinar · 2018-02-03T16:34:00Z

packages/bazel/src/ng_rollup_bundle.bzl

+  # We don't expect anyone to make use of this ES6 bundle, but it makes this rule
+  # compatible with rollup_bundle which allows them to be easily swapped back and
+  # forth.
+  es2015_rollup_config = write_rollup_config(ctx, filename = "_%s.rollup_es6.conf.js")


IgorMinar · 2018-02-03T16:35:27Z

packages/bazel/src/ng_rollup_bundle.bzl

+  # compatible with rollup_bundle which allows them to be easily swapped back and
+  # forth.
+  es2015_rollup_config = write_rollup_config(ctx, filename = "_%s.rollup_es6.conf.js")
+  run_rollup(ctx, collect_es2015_sources(ctx), es2015_rollup_config, ctx.outputs.build_es6)


where is this output defined? I don't see any other references to "build_es6" in this PR

it's in the ROLLUP_OUTPUTS that we inherit from rules_nodejs

IgorMinar · 2018-02-04T13:22:34Z

oops.. closed by mistake. sorry! 😇

mhevery · 2018-02-05T19:26:15Z

.circleci/config.yml

+      # CircleCI will allow us to go back and view/download these artifacts after each build.
+      # Also we can use a service like https://buildsize.org/ to automatically track binary size of these artifacts.
+      - store_artifacts:
+          path: dist/bin/packages/core/test/bundling/hello_world/bundle.min.js


this is no exciting!

mary-poppins · 2018-02-05T22:24:35Z

You can preview 07795d3 at https://pr22004-07795d3.ngbuilds.io/.

mary-poppins · 2018-02-05T23:12:13Z

You can preview 14dfcd9 at https://pr22004-14dfcd9.ngbuilds.io/.

This is a customization of the rollup_bundle rule from rules_nodejs which adds the build-optimizer as a plugin. Add a functional test with fast round-trip that asserts the minified app still works. Publish the min.js artifact on circleCI so we can track its size.

mary-poppins · 2018-02-05T23:25:17Z

You can preview f425a66 at https://pr22004-f425a66.ngbuilds.io/.

This is a customization of the rollup_bundle rule from rules_nodejs which adds the build-optimizer as a plugin. Add a functional test with fast round-trip that asserts the minified app still works. Publish the min.js artifact on circleCI so we can track its size. PR Close angular#22004

angular-automatic-lock-bot · 2019-09-13T06:51:47Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

alexeagle requested review from mhevery and chuckjaz February 2, 2018 23:27

googlebot added the cla: yes label Feb 2, 2018

alexeagle added the target: major This PR is targeted for the next major release label Feb 2, 2018

alexeagle force-pushed the build-optimizer branch from dfcd36d to 16af609 Compare February 3, 2018 00:20

IgorMinar self-requested a review February 3, 2018 00:21

alexeagle force-pushed the build-optimizer branch from 16af609 to 0352989 Compare February 3, 2018 00:27

alexeagle force-pushed the build-optimizer branch 2 times, most recently from b22fcee to 4ac0f20 Compare February 3, 2018 00:39

IgorMinar suggested changes Feb 3, 2018

View reviewed changes

alexeagle force-pushed the build-optimizer branch from 4ac0f20 to 85f77db Compare February 3, 2018 01:09

alexeagle force-pushed the build-optimizer branch from 85f77db to 5110990 Compare February 3, 2018 01:40

IgorMinar reviewed Feb 3, 2018

View reviewed changes

IgorMinar closed this Feb 4, 2018

IgorMinar reopened this Feb 4, 2018

mhevery approved these changes Feb 5, 2018

View reviewed changes

alexeagle force-pushed the build-optimizer branch from 5110990 to 07795d3 Compare February 5, 2018 22:01

alexeagle force-pushed the build-optimizer branch from 07795d3 to 14dfcd9 Compare February 5, 2018 23:05

alexeagle force-pushed the build-optimizer branch from 14dfcd9 to f425a66 Compare February 5, 2018 23:17

alexeagle added the action: merge The PR is ready for merge by the caretaker label Feb 5, 2018

IgorMinar approved these changes Feb 6, 2018

View reviewed changes

alxhub closed this in 370ab66 Feb 6, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

build(ivy): create hello world rollup #22004

build(ivy): create hello world rollup #22004

Conversation

alexeagle commented Feb 2, 2018

mary-poppins commented Feb 2, 2018

mary-poppins commented Feb 3, 2018

mary-poppins commented Feb 3, 2018

mary-poppins commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 3, 2018

mary-poppins commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar commented Feb 4, 2018

Choose a reason for hiding this comment

mary-poppins commented Feb 5, 2018

mary-poppins commented Feb 5, 2018

mary-poppins commented Feb 5, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019