common chunk optional 1.2 update

[coryrylan]

Bug Report or Feature Request (mark with an x)

• bug report -> please search issues before submitting

Versions.

@angular/cli: 1.2.1
node: 8.1.4
os: win32 x64
@angular/animations: 4.1.3
@angular/common: 4.1.3
@angular/compiler: 4.1.3
@angular/core: 4.1.3
@angular/forms: 4.1.3
@angular/http: 4.1.3
@angular/platform-browser: 4.1.3
@angular/platform-browser-dynamic: 4.1.3
@angular/platform-server: 4.1.3
@angular/router: 4.1.3
@angular/cli: 1.2.1
@angular/compiler-cli: 4.1.3
Windows 10

Repro steps.

The commons chunk update that occurred here causes a performance regression 2090f64

The use case, we have three NgModules. Module A, B and C. All three are lazy loaded separately quite large at 50kb each. Module C is lazy loaded but depends on Module A and B. Because Module C depends on A and B the modules are moved into a common chunk and leaves the lazy loaded modules empty with just the module definition code. Now module A, B and C are now all bundled into a common bundle and no longer lazy loaded. Now when user navigates to Feature A all 150kb of feature A, B and C are loaded.

Not sure if there is a fix to this or our best option is to eject. Maybe proposal to make the common chunk optional behind a config? The common bundle removed duplicated code but essentially removed the benefit of lazy loaded modules if one of those happens to depend on each other.

In certain apps with certain use cases having common chunks configurable would be necessary. It would seem that larger apps would likely run into our similar use case.

The log given by the failure.

Desired functionality.

Would be glad to get a pull request to make this optional in the CLI.

Mention any other details that might be useful.

[kevinbuhmann]

An easy fix for this would be to make it possible to disable the common chunk (i.e. use the previous behavior) via a build command arg (alongside --vendor-chunk).

[coryrylan]

This pull request gives the most flexibility I think. It allows the dev to determine the best trade off with their app. More code up front and less duplication vs more duplication and less code up front.

The CommonsChunkPlugin is going to be at odds with the NgModules. I think this could happen with third party code NgModules as well. A third party module is loaded for two of 30 other feature modules. Now all 30 features must aggressively load the two common chunks that only two features need.

[coryrylan]

Additional notes to this case it looks like our project is now running into this issue (6196) after upgrading from cli 1.1 to 1.2 and continues to be broken in both situations on 1.3 beta

It looks like it could be related to the common chunk plugin. Which would make sense because we ran into this issue and the one above at the same time when upgrading to 1.2. If we can get this pull request in this will be able to help with anyone on #6196 as well.

Here is the source issue in webpack
webpack/webpack#959

[coryrylan]

@filipesilva Here is a demo repo you can clone and see the issue. There are three lazy loaded feature modules A, B and C. C relies on A and B so the common chunks removes the A and B components moving them into the common chunk and leaving empty lazy loaded NgModule definitions.

https://github.com/coryrylan/test-proj

[filipesilva]

Thank you for taking the time to do this repro.

I agree that there should be an option to turn it off, but expect that it is overall better to have the common-chunk.

It is true that the first lazy-loaded chunk can be larger than necessary due to dependencies of other lazy-loaded chunks, but it is also true that any chunk but the first will be significantly smaller. Being able to chose is important though.

I don't see the hard connection between this issue and #6196 though. I understand that they might be related from the webpack thread, but it doesn't seem like that's the whole story.

[coryrylan]

Yeah not quite sure how they are related either but just noticed we ran into that same issue at the same time.

On this situation I'm not quite sure what the best thing to do would be for the overall performance health of the app. In our situation having the first load and js smaller is more important as a public facing app. We have the service worker lazy loading those additional modules in the background so having those be a little bigger is not an issue for us. I think it really depends on the app and its goals and what performance trade offs to make.

Ideally being able to adjust the min chunk value would give us the most flexibility because we would like the usefulness of common chunks but not such an aggressive setting value of 2. But I also understand being careful about not exposing to many webpack settings in the public api.

[clydin]

I definitely agree on adding an option flag for the behavior.
Something to note though is that this common chunk should not be loading eagerly. It should be loaded asynchronously only when an async chunk requires one of the moved dependencies. Granted this is still not ideal in this situation but if this behavior is not being observed than further investigation should probably be done.

Also, looking at the repro (thanks btw), as more of a general guideline, I would recommend not reusing lazy routing modules as direct dependencies and instead split out commonly used components/pipes/etc. into either sub-feature modules or shared common modules. This should help remove some final code duplication as well as simplify the overall dependency hierarchy.

[coryrylan]

The issue with the above example repo is that this can happen with third party code as well. For example our app uses a very heavy maps module 300kb. If only two features out of 50 use it now all 50 are paying the penalty of the large plugin that is not used very often.

Even with our own code feature C is a combination of feature A and B so if we put all the shared code in a common module it defeats the purpose and lazy loading benefits are minimal. We already have a shared module for shared components and for large apps this is very difficult to get right. If we put to much code in shared then every feature on the site pays a penalty. We would rather have some runtime duplication over a heavy initial payload bundle. Unfortunately most demo apps never demonstrate this issue because they are not large enough to deal with these kind of dependencies. This is a issue I see across all types of JS apps not just Angular. Its a difficult problem to solve.

[kevinbuhmann]

To be more specific, in @coryrylan's example, Module A is a WigetsAModule, Module B is a WidgetsBModule, and Module C is a SearchModule. The search page (in SearchModule) displays both kinds of widgets, but pulling the common code that search uses would slow down our widget pages by loading another module. That's not what we want.

[filipesilva]

Heya @coryrylan and @kevinphelps, I ran this problem by the team today and the way we'd like to address it is to add the --common-chunk flag, but not the --common-chunk-min-chunks one.

We want to enable you to disable this feature as it was added without any way to control it. But it's purpose is not only payload optimization though. It also addresses the problem of library duplication, especially for cases where individual instances matter. That was a bug we had in lazy chunks and the common chunk fixes it.

We'd like to get #7023 in (with the modifications I mentioned) either tomorrow or thursday, so that it can be in this week's release (1.3.0-rc.0). If it doesn't get on rc, it will only be available on 1.4.0-beta.0 since it's a feature.

@kevinphelps do you have time to work on the PR? All it needs is to remove the second flag and to add another one of these bits https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/misc/common-async.ts#L44-L51 to the test to ensure that when the flag is on, the number of files is the same as before.

[filipesilva]

Also, in the future we want to add some configuration items to .angular-cli.json to enable prod optimizations more granularly so you don't need to be configuring it all with flags.

[kevinbuhmann]

I was wondering if I should modify .angular-cli.json. I can help work on that if you want to keep me in the loop.

[coryrylan]

We should be able to get that pull request done today to make it in for 1.3.0-rc0

[filipesilva]

Yes, that option can go there as well. A good example of how it can be added is #7007. This PR also adds a single flag to the cli json. The important bit is loading the default and using it in packages/@angular/cli/commands/build.ts.

[kevinbuhmann]

@filipesilva Should the --vendor-chunk option be in the .angular-cli.json build config also?

[filipesilva]

Perhaps, but that's one that I'll add myself in another PR, I have to fiddle with that logic for another reason. Don't worry about it for your PR.

[kevinbuhmann]

okay cool

[kevinbuhmann]

Thanks!

[keatkeat87]

cool, i just want this

[adamdport]

TL;DR for CLI 1.x: ng serve --common-chunk=false or in your .angular-cli.json:

{ 
  "defaults": {
    "styleExt": "scss",
    ...
    "build": {
      "commonChunk": false
    }
  }
}

[stephengardner]

Thanks @adamdport , is this documented anywhere?

And are there more options regarding the commons chunk -- such as "Only add something to the commons chunk if it is found in X or more feature modules" ?

[adamdport]

@stephengardner That's #8458, smash that like button

[angular-automatic-lock-bot]

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