Update Angular CLI ivy tests by filipesilva · Pull Request #28372 · angular/angular

filipesilva · 2019-01-25T16:34:05Z

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?

This PR updates the versions of Angular CLI and Build Optimizer used in Angular tests.

The updated versions contain improvements in code optimization that enable smaller bundle sizes with Ivy on Angular CLI and Bazel projects.

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

cexbrayat · 2019-01-25T16:45:24Z

@filipesilva Dumb question: if this enables smaller bundles, shouldn't https://github.com/angular/angular/blob/master/integration/_payload-limits.json be updated with smaller numbers for cli-hello-world-ivy?

filipesilva · 2019-01-25T16:47:41Z

@cexbrayat not a dumb question at all, but rather a very valid point I did not address. Thanks for a heads up!

JoostK · 2019-01-28T17:30:34Z

Amazing work! 🎉

gkalpak

🎉

gkalpak · 2019-01-28T18:02:07Z

+ cli-hello-world-ivy

mary-poppins · 2019-01-29T11:52:05Z

You can preview a2e7fb6 at https://pr28372-a2e7fb6.ngbuilds.io/.

mary-poppins · 2019-01-29T12:31:26Z

You can preview 9d7620e at https://pr28372-9d7620e.ngbuilds.io/.

mary-poppins · 2019-01-29T13:15:46Z

You can preview a943c32 at https://pr28372-a943c32.ngbuilds.io/.

mary-poppins · 2019-01-29T14:18:41Z

You can preview 82f3686 at https://pr28372-82f3686.ngbuilds.io/.

gkalpak

The permission changes (100755 --> 100644) in integration/cli-hello-world-ivy/debug-test.sh should be reverted (afaict).

gkalpak · 2019-01-29T14:18:00Z

cli-hello-world-ivy --> cli-hello-world-ivy-compat

gkalpak · 2019-01-29T14:19:34Z

CliHelloWorldIvy --> CliHelloWorldIvyCompat

gkalpak · 2019-01-29T14:24:51Z

cli-hello-world --> cli-hello-world-ivy

gkalpak · 2019-01-29T14:26:19Z

Why not add a pipe 😉

This integration test is meant to mimic https://github.com/angular/angular/tree/master/packages/core/test/bundling/hello_world and obtain similar sizes bundles. For that reason it doesn't have anything more than that app does.

I suppose this isn't clear because I have mentioned it exactly 0 times, and nothing in the test name reflects that either.

Maybe this test should be called cli-minimal-ivy-hello-world, WDYT? And reference that it's a version of that other one somewhere too.

I see. It is definitely worth mentioning somewhere, because my general assumption is that cli-* projects in integration/ are sort of equivalent to each other (which would make much more sense imo, especially given that their tracked payload sizes are included side-by-side in the same JSON file).

But if this one must much the one in packages/, your suggestions SGTM.

Renamed to cli-hello-world-ivy-minimal, added a note to the integration test README, and to the app main.ts.

gkalpak · 2019-01-29T14:26:42Z

Why such a minimal template?

Same answer as the pipe comment above.

gkalpak · 2019-01-29T14:31:03Z

Maybe we could log a warning for now? Or is it safe to always return all lazy routes?

I wouldn't like a warning because there's no way to turn it off.

I think it's ok to always return all lazy routes but will confirm with the team.

Suggested change

// Listing specific routes is unsupported for now.

// Listing specific routes is unsupported for now, so we erroneously

// return all lazy routes instead (which should be okay for the CLI's

// usage).

mary-poppins · 2019-01-30T14:07:47Z

You can preview 6dbcf85 at https://pr28372-6dbcf85.ngbuilds.io/.

filipesilva · 2019-01-30T14:08:33Z

@gkalpak the permissions change should be gone now.

mary-poppins · 2019-01-30T14:10:55Z

You can preview cf3ecbc at https://pr28372-cf3ecbc.ngbuilds.io/.

mary-poppins · 2019-01-30T14:12:50Z

You can preview e513ab5 at https://pr28372-e513ab5.ngbuilds.io/.

mary-poppins · 2019-01-30T14:34:26Z

You can preview 778bea7 at https://pr28372-778bea7.ngbuilds.io/.

mary-poppins · 2019-01-30T14:58:52Z

You can preview 5507426 at https://pr28372-5507426.ngbuilds.io/.

gkalpak

👌

gkalpak · 2019-02-02T00:32:41Z

CliHelloWorldIvy --> CliHelloWorldIvyMinimal 😇

Donesies 👍

mary-poppins · 2019-02-05T16:51:47Z

You can preview 61d6de9 at https://pr28372-61d6de9.ngbuilds.io/.

mary-poppins · 2019-02-05T17:44:10Z

You can preview 4162517 at https://pr28372-4162517.ngbuilds.io/.

mary-poppins · 2019-02-05T19:11:28Z

You can preview 1def325 at https://pr28372-1def325.ngbuilds.io/.

mary-poppins · 2019-02-06T09:13:33Z

You can preview be6ef37 at https://pr28372-be6ef37.ngbuilds.io/.

alxhub · 2019-02-06T15:17:44Z

Suggested change

// Listing specific routes is unsupported for now.

// Listing specific routes is unsupported for now, so we erroneously

// return all lazy routes instead (which should be okay for the CLI's

// usage).

…route In #27697 the listLazyRoutes was fixed to work with ivy. Since the entryRoute argument is not supported, it was made to also error. But by erroring it breaks existing usage with Angular CLI where the entry route is sent in as an argument. This commit changes listLazyRoutes to not error out, but instead ignore the argument.

There are two ways to bootstrap an Ivy app: with `platformBrowserDynamic().bootstrapModule` and `renderComponent`. To distinguish between these two approaches we call the `platformBrowserDynamic().bootstrapModule` way `ivy-compat` and the `renderComponent` way just `ivy`.

mary-poppins · 2019-02-06T15:39:46Z

You can preview 4b9641b at https://pr28372-4b9641b.ngbuilds.io/.

…ts (#28372) PR Close #28372

…28372) There are two ways to bootstrap an Ivy app: with `platformBrowserDynamic().bootstrapModule` and `renderComponent`. To distinguish between these two approaches we call the `platformBrowserDynamic().bootstrapModule` way `ivy-compat` and the `renderComponent` way just `ivy`. PR Close #28372

) PR Close #28372

PR Close #28372

angular-automatic-lock-bot · 2019-09-14T16:22:27Z

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

filipesilva requested review from a team January 25, 2019 16:34

googlebot added the cla: yes label Jan 25, 2019

filipesilva added the target: major This PR is targeted for the next major release label Jan 25, 2019

filipesilva requested a review from gkalpak January 25, 2019 16:36

jasonaden added the area: build & ci Related the build and CI infrastructure of the project label Jan 25, 2019

ngbot Bot added this to the needsTriage milestone Jan 25, 2019

filipesilva changed the title ~~build: update version of Angular CLI and Build Optimizer used for tests~~ Update Angular CLI ivy tests Jan 28, 2019

JoostK reviewed Jan 28, 2019

View reviewed changes

Comment thread integration/_payload-limits.json Outdated

JoostK Jan 28, 2019

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! 🎉

gkalpak reviewed Jan 28, 2019

View reviewed changes

filipesilva requested a review from a team January 29, 2019 11:47

filipesilva requested a review from gkalpak January 29, 2019 14:15

gkalpak reviewed Jan 29, 2019

View reviewed changes

filipesilva requested review from alexeagle, alxhub and gkalpak January 29, 2019 14:34

gkalpak added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 2, 2019

gkalpak approved these changes Feb 2, 2019

View reviewed changes

filipesilva mentioned this pull request Feb 5, 2019

Ivy Snapshots e2e job angular/angular-cli#13597

Merged

alxhub approved these changes Feb 6, 2019

View reviewed changes

filipesilva added 6 commits February 6, 2019 15:35

build: update version of Angular CLI and Build Optimizer used for tests

9250f86

build: add cli-hello-world-ivy-minimal test with renderComponent

597b895

build: re-enable CLI integration test for prod mode e2e

e4ec24f

build: update render3 section of integration test readme

4b9641b

alexeagle approved these changes Feb 7, 2019

View reviewed changes

alexeagle added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 7, 2019

mhevery closed this in 2caa419 Feb 7, 2019

mhevery pushed a commit that referenced this pull request Feb 7, 2019

build: update version of Angular CLI and Build Optimizer used for tes…

ce83231

…ts (#28372) PR Close #28372

mhevery pushed a commit that referenced this pull request Feb 7, 2019

build: add cli-hello-world-ivy-minimal test with renderComponent (#28372

df3cea4

) PR Close #28372

mhevery pushed a commit that referenced this pull request Feb 7, 2019

build: re-enable CLI integration test for prod mode e2e (#28372)

f889317

PR Close #28372

mhevery pushed a commit that referenced this pull request Feb 7, 2019

build: update render3 section of integration test readme (#28372)

264ef72

PR Close #28372

filipesilva deleted the cli-ivy-updates branch February 7, 2019 21:10

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

-    // Listing specific routes is unsupported for now.
+    // Listing specific routes is unsupported for now, so we erroneously
+    // return all lazy routes instead (which should be okay for the CLI's
+    // usage).

Conversation

filipesilva commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

cexbrayat commented Jan 25, 2019

Uh oh!

filipesilva commented Jan 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

gkalpak Jan 28, 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!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jan 29, 2019

Uh oh!

mary-poppins commented Jan 29, 2019

Uh oh!

mary-poppins commented Jan 29, 2019

Uh oh!

mary-poppins commented Jan 29, 2019

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

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!

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!

mary-poppins commented Jan 30, 2019

Uh oh!

filipesilva commented Jan 30, 2019

Uh oh!

mary-poppins commented Jan 30, 2019

Uh oh!

mary-poppins commented Jan 30, 2019

Uh oh!

mary-poppins commented Jan 30, 2019

Uh oh!

mary-poppins commented Jan 30, 2019

filipesilva commented Jan 25, 2019 •

edited

Loading

gkalpak Jan 28, 2019 •

edited

Loading