feat(bazel): update bazel-schematics to use Ivy and new rollup_bundle #33435

gregmagolan · 2019-10-27T20:10:30Z

Note: the @angular/bazel schematic now appends the package.json "script" field with 'ngcc --properties es2015 browser module main'. If there is an existing script field with ngcc then the schematic modifies it in place removing --first-only and --create-ivy-entry-points.

ViewEngine sources under node_modules need to be updated in-place for Bazel as it does not know how to use the __ivy__ entry points that are created by the non-bazel ngcc command that is added to "scripts" :ngcc --properties es2015 browser module main --first-only --create-ivy-entry-points.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

@angular/bazel schematics was depending on the legacy rollup_bundle rule which will be removed for the nodejs rules 1.0 release and it was building ViewEngine

What is the new behavior?

@angular/bazel schematics updated to Ivy & to the new rollup_bundle rule

Does this PR introduce a breaking change?

Yes
No

Other information

alexeagle · 2019-10-28T14:26:44Z

packages/bazel/src/schematics/ng-add/index.ts

    if (scripts) {
      const postInstall = findPropertyInAstObject(scripts, 'postinstall');
      if (postInstall) {
-        const command = `${postInstall.value}; ${ngcCommand}`;
+        // Our `ngcc` command should go first so it is executed before before the CLI
+        // generated ngcc command as if it goes after it does not work as expected


that's strange that you could end up with two ngcc commands?

ng new with ivy adds "scripts": { "postinstall": "ngcc --properties es2015 browser module main --first-only --create-ivy-entry-points" } which doesn't work with bazel because of --first-only and --create-ivy-entry-points

chatted with @filipesilva about this on slack and I think the better solution is to mutate the ngcc <options> to just ngcc in the bazel schematic instead of adding a second

Even with just ngcc, there are still two ngcc commands aren't there?
I think Alex's point was, why not just one ngcc command?

Cleaned up. If there is an existing ngcc command then the schematic modified the one in-place removing --first-only and --create-ivy-entry-points:

if (/\bngcc\b/.test(value)) { // `ngcc` is already in the postinstall script value = value .replace(/\s*--first-only\b/, '') .replace(/\s*--create-ivy-entry-points\b/, ''); replacePropertyInAstObject(recorder, scripts, 'postinstall', value); } else { const command = `${postInstall.value}; ${ngccCommand}`; replacePropertyInAstObject(recorder, scripts, 'postinstall', command); }

alexeagle · 2019-10-28T14:27:41Z

/cc @kyliau

josephperrott

This also moves to bazel 1.1.0? We should definitely be noting that in the commits/PR message.

gregmagolan · 2019-10-28T14:41:42Z

This also moves to bazel 1.1.0? We should definitely be noting that in the commits/PR message.

👍 will add that note and cleanup the ngcc postinstall

gregmagolan · 2019-10-28T15:27:19Z

Actually, given bazelbuild/rules_nodejs#1307 I'll remove the Bazel update on this PR as it will break Windows users.

kyliau

LGTM, thank you!

josephperrott

LGTM

alexeagle · 2019-10-29T15:36:35Z

Caretaker: assistance because there is a "cla/google" status that isn't turning green

josephperrott · 2019-10-29T15:49:54Z

Removing merge-assistance as cla/google status is corrected

Note: the @angular/bazel schematic now appends the package.json "script" field with 'ngcc --properties es2015 browser module main'. If there is an existing script field with ngcc then the schematic modifies it in place removing `--first-only` and `--create-ivy-entry-points`. ViewEngine sources under node_modules need to be updated in-place for Bazel as it does not know how to use the `__ivy__` entry points that are created by the non-bazel `ngcc` command that is added to "scripts" :`ngcc --properties es2015 browser module main --first-only --create-ivy-entry-points`.

gregmagolan · 2019-10-29T17:24:07Z

@caretaker Noticed the comment on the commit was outdated so updated it as it's a feat commit. No other changes. CI running again.

…angular#33435) Note: the @angular/bazel schematic now appends the package.json "script" field with 'ngcc --properties es2015 browser module main'. If there is an existing script field with ngcc then the schematic modifies it in place removing `--first-only` and `--create-ivy-entry-points`. ViewEngine sources under node_modules need to be updated in-place for Bazel as it does not know how to use the `__ivy__` entry points that are created by the non-bazel `ngcc` command that is added to "scripts" :`ngcc --properties es2015 browser module main --first-only --create-ivy-entry-points`. PR Close angular#33435

angular-automatic-lock-bot · 2019-11-29T16:11:10Z

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.}

gregmagolan requested a review from alexeagle October 27, 2019 20:10

gregmagolan requested review from a team as code owners October 27, 2019 20:10

googlebot added the cla: yes label Oct 27, 2019

gregmagolan force-pushed the bazel_schematics_ivy branch from 76716dc to 984164e Compare October 27, 2019 20:14

gregmagolan mentioned this pull request Oct 27, 2019

build: remove deps on legacy nodejs rules rollup_bundle internals #33201

Closed

alexeagle added the target: major This PR is targeted for the next major release label Oct 28, 2019

alexeagle approved these changes Oct 28, 2019

View reviewed changes

josephperrott reviewed Oct 28, 2019

View reviewed changes

gregmagolan added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 28, 2019

gregmagolan force-pushed the bazel_schematics_ivy branch 3 times, most recently from c75a576 to 2261c57 Compare October 28, 2019 22:38

gregmagolan requested review from kyliau and josephperrott October 28, 2019 22:39

gregmagolan removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 28, 2019

kyliau approved these changes Oct 28, 2019

View reviewed changes

kyliau added the area: bazel Issues related to the published `@angular/bazel` build rules label Oct 28, 2019

ngbot bot added this to the needsTriage milestone Oct 28, 2019

josephperrott approved these changes Oct 29, 2019

View reviewed changes

alexeagle added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 29, 2019

josephperrott added cla: yes and removed cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 29, 2019

gregmagolan force-pushed the bazel_schematics_ivy branch from 2261c57 to c90303a Compare October 29, 2019 17:22

AndrewKushnir closed this in bf913cc Oct 29, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bazel): update bazel-schematics to use Ivy and new rollup_bundle #33435

feat(bazel): update bazel-schematics to use Ivy and new rollup_bundle #33435

gregmagolan commented Oct 27, 2019 •

edited

alexeagle Oct 28, 2019

gregmagolan Oct 28, 2019 •

edited

kyliau Oct 28, 2019

gregmagolan Oct 28, 2019 •

edited

alexeagle commented Oct 28, 2019

josephperrott left a comment

gregmagolan commented Oct 28, 2019

gregmagolan commented Oct 28, 2019

kyliau left a comment

josephperrott left a comment

alexeagle commented Oct 29, 2019

josephperrott commented Oct 29, 2019

gregmagolan commented Oct 29, 2019

angular-automatic-lock-bot bot commented Nov 29, 2019

feat(bazel): update bazel-schematics to use Ivy and new rollup_bundle #33435

feat(bazel): update bazel-schematics to use Ivy and new rollup_bundle #33435

Conversation

gregmagolan commented Oct 27, 2019 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle Oct 28, 2019

Choose a reason for hiding this comment

gregmagolan Oct 28, 2019 • edited

Choose a reason for hiding this comment

kyliau Oct 28, 2019

Choose a reason for hiding this comment

gregmagolan Oct 28, 2019 • edited

Choose a reason for hiding this comment

alexeagle commented Oct 28, 2019

josephperrott left a comment

Choose a reason for hiding this comment

gregmagolan commented Oct 28, 2019

gregmagolan commented Oct 28, 2019

kyliau left a comment

Choose a reason for hiding this comment

josephperrott left a comment

Choose a reason for hiding this comment

alexeagle commented Oct 29, 2019

josephperrott commented Oct 29, 2019

gregmagolan commented Oct 29, 2019

angular-automatic-lock-bot bot commented Nov 29, 2019

gregmagolan commented Oct 27, 2019 •

edited

gregmagolan Oct 28, 2019 •

edited

gregmagolan Oct 28, 2019 •

edited