test: track payload size for cli-hello-world-ivy by cexbrayat · Pull Request #27797 · angular/angular

cexbrayat · 2018-12-21T13:10:38Z

Adds size bundle tracking to the integration test cli-hello-world-ivy and updates the app itself to reflect the result of using the experimentalIvy flag on the CLI.
The result is similar to:

npx @angular/cli@next new cli-hello-world-ivy --experimental-ivy --defaults

But replaces the current (cli 7.2.0-rc.0) renderComponent bootstrap with the usual platformBrowserDynamic one.
It also keeps what the app did (display a pipe, tests it).

This has been added to track regressions in bundle sizes of a CLI app with Ivy enabled.

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:

Updates an integration test and adds tracking for the bundle size of the CLI app with Ivy enabled (which is currently weirdly high at 496K).

Does this PR introduce a breaking change?

Yes
No

Other information

This has been discussed on Slack with @petebacondarwin @IgorMinar and @alan-agius4

cexbrayat · 2018-12-21T13:13:48Z

I don't think this is useful anymore, but maybe I'm mistaken.

I believe that this is now handled by CLI

Yup. It shouldn't be needed. OK to drop.

JoostK · 2018-12-21T14:36:47Z

Nice! The current payload size is so high because platformBrowserDynamic pulls in a lot of ViewEngine together with the Compiler infrastructure. If you switch to platformBrowser it will become significantly smaller (but still quite large). I wonder if the CLI replaces platformBrowserDynamic with platformBrowser during prod builds? Is the prod build actually tested here?

cexbrayat · 2018-12-21T15:21:51Z

@JoostK thanks! Yes it should be the prod build. I don't know where the issue is, but we indeed end up with the compiler in the bundle (discussion in ivy-cli-build-size on Slack).

JoostK · 2018-12-21T17:34:21Z

@cexbrayat It may be a good idea to verify that native ngtsc behaves the same. The cli-hello-world-ivy app uses ngcc to convert the packages_dist output, perhaps there's currently some discrepancies between its output when compared to native ngtsc builds.

gkalpak

Thx for the updates 🎉

Basically, this PR contains two changes (that should be in two separate commits imo):

Update the cli-hello-world-ivy integration project to latest CLI.
Enable payload-size checking for the same project.

The former looks great. I am not convinced about the latter at this point.
What is the reasoning for adding this check now, given that Ivy is still not finalized and bundle sizes are may change (hopefully downwards).

gkalpak · 2018-12-21T17:29:35Z

Does moving this here form tsconfig.json make a difference?

I don't think it really does, and this how experimentalIvy currently generates it

The reason being that it’s here is that you can have multiple projects within a workspace which don’t necessarily use ivy.

That said for this case, there is no difference.

cexbrayat · 2018-12-21T17:47:04Z

@gkalpak I can split in two commits if needed sure.

We added the size tracking because the result is currently weird (the bundle contains the compiler) after discussing it with Igor on Slack

gkalpak · 2018-12-21T17:47:24Z

Also note that BuildOptimizer currently does not work correctly with platformBrowserDynamic() (see angular/angular-cli#13261), so as long as the prod build is broken, it makes even less sense to check payload sizes imo.

cexbrayat · 2018-12-21T17:50:13Z

Yeah we found out when digging out this issue :)

googlebot · 2018-12-21T18:03:23Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-21T18:07:06Z

CLAs look good, thanks!

cexbrayat · 2018-12-21T18:08:58Z

@gkalpak I splitted into two commits

gkalpak

The first commit seems more like build: ... (rather than test: ...). I would also be better to mention the cli version explicitly (e.g. build: update `cli-hello-world-ivy` to cli@...).

The second commit message mentions "track payload size", but the change does not really track it (in the sense that it saves it to Firebase and we can see how it evolves over time). It rather checks the payload size and fails the build if it deviates more than 1% percent from the set limits).
You might want to be more explicit on that in the commit message to avoid confusion in the future.

Also, if the intention was to actually record the size changes in Firebase, then you need more changes 😃

The code LGTM. I am still not convinced it adds much benefit to check the size when there are known bugs that affect bundling, but if @IgorMinar thinks so then it probably is so 😁

cexbrayat · 2018-12-22T15:45:53Z

@gkalpak thanks for the feedback, I updated the commit messages 👍 I admit that I don't know the build setup well enough to add the necessary for recording the sizes in Firebase, but as you say that's probably not necessary right now.

gkalpak · 2018-12-23T10:30:37Z

(FWIW, I'd be happy to help setting that up, if/when we decide it makes sense to do so.)

IgorMinar

This looks great! Thanks so much for getting it done!

IgorMinar · 2018-12-25T16:28:30Z

@gkalpak the goal of this test is to prevent further regressions. We know that things are broken now, but as we fix them we need to prevent reintroducing more issues down the line.

IgorMinar · 2018-12-25T16:32:14Z

As far as test/build/ci commit message types goes I'd categorize this as ci or test. See my definition of types at https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit

Having said that, the current pr is "good enough". But I won't stop you from making it perfect. 😉

cexbrayat · 2018-12-25T16:36:39Z

@IgorMinar Haha, changing a commit message is not the hardest part ;) I'll revert from build to test.

Updates the app itself to reflect the result of using the `experimentalIvy` flag on the CLI. The result is similar to: npx @angular/cli@next new cli-hello-world-ivy --experimental-ivy --defaults But replaces the current (cli `7.2.0-rc.0`) `renderComponent` bootstrap with the usual `platformBrowserDynamic` one. It also keeps what the app did (display a pipe, tests it).

Adds size bundle checking to the integration test `cli-hello-world-ivy`

Adds size bundle checking to the integration test `cli-hello-world-ivy` PR Close #27797

gkalpak · 2018-12-27T16:52:08Z

As far as test/build/ci commit message types goes I'd categorize this as ci or test. See my definition of types at https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit

The PR had 2 commits (one updating the project to the latest cli and one updating the tests to check for payload size changes).
The way I understand the type descriptions, I would still categorize the 1st commit as build (and the 2nd as test).
Is my understandign wrong? Is it because the cli-hello-world-ivy project is used for testing purposes, so everything related to that is a test change?

(@IgorMinar, asking to make sure we are on the same page and that I use appropriate types in my commit messages.)

angular-automatic-lock-bot · 2019-09-14T11:51:42Z

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

googlebot added the cla: yes label Dec 21, 2018

cexbrayat commented Dec 21, 2018

View reviewed changes

gkalpak reviewed Dec 21, 2018

View reviewed changes

gkalpak added area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD labels Dec 21, 2018

ngbot Bot added this to the needsTriage milestone Dec 21, 2018

cexbrayat force-pushed the test/ivy-cli-bundle-size branch from 3955819 to 7f3eab2 Compare December 21, 2018 18:03

googlebot added cla: no and removed cla: yes labels Dec 21, 2018

cexbrayat force-pushed the test/ivy-cli-bundle-size branch from 7f3eab2 to 06e4cf1 Compare December 21, 2018 18:07

googlebot added cla: yes and removed cla: no labels Dec 21, 2018

gkalpak approved these changes Dec 22, 2018

View reviewed changes

cexbrayat force-pushed the test/ivy-cli-bundle-size branch from 06e4cf1 to 542f8e1 Compare December 22, 2018 15:43

IgorMinar approved these changes Dec 25, 2018

View reviewed changes

IgorMinar added the action: merge The PR is ready for merge by the caretaker label Dec 25, 2018

IgorMinar added target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD labels Dec 25, 2018

cexbrayat added 2 commits December 25, 2018 17:37

test: check payload size for cli-hello-world-ivy

748471e

Adds size bundle checking to the integration test `cli-hello-world-ivy`

cexbrayat force-pushed the test/ivy-cli-bundle-size branch from 542f8e1 to 748471e Compare December 25, 2018 16:37

benlesh closed this in 62e45ce Dec 26, 2018

benlesh pushed a commit that referenced this pull request Dec 26, 2018

test: check payload size for cli-hello-world-ivy (#27797)

3f64e87

Adds size bundle checking to the integration test `cli-hello-world-ivy` PR Close #27797

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

Conversation

cexbrayat commented Dec 21, 2018

PR Checklist

PR Type

Does this PR introduce a breaking change?

Other information

Uh oh!

cexbrayat Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

IgorMinar Dec 25, 2018

Choose a reason for hiding this comment

Uh oh!

JoostK commented Dec 21, 2018

Uh oh!

cexbrayat commented Dec 21, 2018

Uh oh!

JoostK commented Dec 21, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

gkalpak Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

cexbrayat Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Dec 21, 2018 • 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!

cexbrayat commented Dec 21, 2018

Uh oh!

gkalpak commented Dec 21, 2018

Uh oh!

cexbrayat commented Dec 21, 2018

Uh oh!

googlebot commented Dec 21, 2018

Uh oh!

googlebot commented Dec 21, 2018

Uh oh!

cexbrayat commented Dec 21, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

cexbrayat commented Dec 22, 2018

Uh oh!

gkalpak commented Dec 23, 2018

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Dec 25, 2018

Uh oh!

IgorMinar commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cexbrayat commented Dec 25, 2018

Uh oh!

gkalpak commented Dec 27, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alan-agius4 Dec 21, 2018 •

edited

Loading

IgorMinar commented Dec 25, 2018 •

edited

Loading