-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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. |
filipesilva
left a comment
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. |
filipesilva
left a comment
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