New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: track payload size for cli-hello-world-ivy #27797
Conversation
<base href="/"> | ||
|
||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<link rel="icon" type="image/x-icon" href="favicon.ico"> | ||
<script>window['ngDevMode'] = true;</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is useful anymore, but maybe I'm mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is now handled by CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. It shouldn't be needed. OK to drop.
Nice! The current payload size is so high because |
@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 |
@cexbrayat It may be a good idea to verify that native ngtsc behaves the same. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
], | ||
"angularCompilerOptions": { | ||
"enableIvy": "ngtsc" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does moving this here form tsconfig.json
make a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it really does, and this how experimentalIvy
currently generates it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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 |
Also note that BuildOptimizer currently does not work correctly with |
Yeah we found out when digging out this issue :) |
3955819
to
7f3eab2
Compare
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 |
7f3eab2
to
06e4cf1
Compare
CLAs look good, thanks! |
@gkalpak I splitted into two commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😁
06e4cf1
to
542f8e1
Compare
@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. |
(FWIW, I'd be happy to help setting that up, if/when we decide it makes sense to do so.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks so much for getting it done!
@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. |
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. 😉 |
@IgorMinar Haha, changing a commit message is not the hardest part ;) I'll revert from |
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`
542f8e1
to
748471e
Compare
Adds size bundle checking to the integration test `cli-hello-world-ivy` PR Close #27797
The PR had 2 commits (one updating the project to the latest cli and one updating the tests to check for payload size changes). (@IgorMinar, asking to make sure we are on the same page and that I use appropriate types in my commit messages.) |
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). PR Close angular#27797
Adds size bundle checking to the integration test `cli-hello-world-ivy` PR Close angular#27797
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). PR Close angular#27797
Adds size bundle checking to the integration test `cli-hello-world-ivy` PR Close angular#27797
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds size bundle tracking to the integration test
cli-hello-world-ivy
and updates the app itself to reflect the result of using theexperimentalIvy
flag on the CLI.The result is similar to:
But replaces the current (cli
7.2.0-rc.0
)renderComponent
bootstrap with the usualplatformBrowserDynamic
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:
PR Type
What kind of change does this PR introduce?
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?
Other information
This has been discussed on Slack with @petebacondarwin @IgorMinar and @alan-agius4