-
Notifications
You must be signed in to change notification settings - Fork 12k
feat(@angular/cli): make the common chunk optional #7023
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
Conversation
I need guidance on testing this. The default project doesn't generate a common chunk, so I couldn't find anything to test against. |
0326db5
to
8a324f3
Compare
Heya @kevinphelps, thanks for taking this on. I can't promise it will go in as is (flag names, functionality), but for now it looks good and I will bring it up for discussion. As for testing, you can look at https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/misc/common-async.ts for the existing test. Another similar test is https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/misc/lazy-module.ts. In both we generate lazy modules and then count output files. Everything is automatically cleaned up after the test is done. |
Thanks! I wasn't sure if the team would accept the I'll take a look those tests and add a test for this option. |
Yeah I definitely understand the concern exposed in #7021, it's a big problem for bigger apps. |
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.
Can you remove the --common-chunk-min-chunks
flag and add a test for --no-common-chunk
in https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/misc/common-async.ts#L44-L51 please?
See #7021 (comment) for context.
I have made the changes locally. I will update this PR once the full test suite completes. |
8a324f3
to
64273bd
Compare
- Add a "--no-common-chunk" build option for disabling the common async chunk. - The existing behavior is maintained by default. closes angular#7021
64273bd
to
9e42ebe
Compare
The tests passed in https://travis-ci.org/angular/angular-cli/jobs/255060001. I only changed the commit message. It looks like a rebuild is needed because Chrome timed out. |
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.
LGTM, thanks a lot for your work in adding this option 👍
No problem! |
Thanks! |
On Wed, Jul 19, 2017 at 8:27 AM Kevin Phelps ***@***.***> wrote:
Thanks!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7023 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIBfe1-LJGk11RSKR-3eLBfze16GX9aLks5sPhJVgaJpZM4Oa2F7>
.
--
Sent from Gmail Mobile
|
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. |
closes #7021