Skip to content

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

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

kevinbuhmann
Copy link
Contributor

@kevinbuhmann kevinbuhmann commented Jul 18, 2017

  • Add a "--no-common-chunk" build option for disabling the common async chunk.
  • The existing behavior is maintained by default.

closes #7021

@kevinbuhmann
Copy link
Contributor Author

kevinbuhmann commented Jul 18, 2017

I need guidance on testing this. The default project doesn't generate a common chunk, so I couldn't find anything to test against.

@kevinbuhmann kevinbuhmann force-pushed the optional-common-chunk branch from 0326db5 to 8a324f3 Compare July 18, 2017 05:00
@filipesilva
Copy link
Contributor

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.

@kevinbuhmann
Copy link
Contributor Author

kevinbuhmann commented Jul 18, 2017

Thanks! I wasn't sure if the team would accept the --common-chunk-min-chunks option, but I added it in case. I think it makes it more flexible, but I understand that it might expose too much of the webpack config in the public api. But it would great the --common-chunk/--no-common-chunk option could be accepted so that apps could use the previous functionality. :)

I'll take a look those tests and add a test for this option.

@filipesilva
Copy link
Contributor

Yeah I definitely understand the concern exposed in #7021, it's a big problem for bigger apps.

Copy link
Contributor

@filipesilva filipesilva left a 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.

@kevinbuhmann kevinbuhmann changed the title feat(@angular/cli): make the common chunk optional/configurable feat(@angular/cli): make the common chunk optional Jul 18, 2017
@kevinbuhmann
Copy link
Contributor Author

I have made the changes locally. I will update this PR once the full test suite completes.

@kevinbuhmann kevinbuhmann force-pushed the optional-common-chunk branch from 8a324f3 to 64273bd Compare July 18, 2017 23:02
- Add a "--no-common-chunk" build option for disabling the common async chunk.
- The existing behavior is maintained by default.

closes angular#7021
@kevinbuhmann kevinbuhmann force-pushed the optional-common-chunk branch from 64273bd to 9e42ebe Compare July 18, 2017 23:44
@kevinbuhmann
Copy link
Contributor Author

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.

Copy link
Contributor

@filipesilva filipesilva left a 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 👍

@filipesilva filipesilva self-assigned this Jul 19, 2017
@kevinbuhmann
Copy link
Contributor Author

No problem!

@Brocco Brocco merged commit 26e9433 into angular:master Jul 19, 2017
@kevinbuhmann
Copy link
Contributor Author

Thanks!

@bossysmile11
Copy link

bossysmile11 commented Jul 19, 2017 via email

@kevinbuhmann kevinbuhmann deleted the optional-common-chunk branch July 21, 2017 18:39
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

common chunk optional 1.2 update
5 participants