Skip to content
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

NG_VALUE_ACCESSOR as a separate variable retains dead code in Angular CLI 11 #19572

Closed
1 of 15 tasks
devoto13 opened this issue Dec 8, 2020 · 4 comments · Fixed by #19573
Closed
1 of 15 tasks

NG_VALUE_ACCESSOR as a separate variable retains dead code in Angular CLI 11 #19572

devoto13 opened this issue Dec 8, 2020 · 4 comments · Fixed by #19573

Comments

@devoto13
Copy link
Contributor

devoto13 commented Dec 8, 2020

🐞 Bug report

Command (mark with an x)

  • new
  • build
  • serve
  • test
  • e2e
  • generate
  • add
  • update
  • lint
  • extract-i18n
  • run
  • config
  • help
  • version
  • doc

Is this a regression?

Yes. It is not present in the fresh project generated with Angular CLI 10.

Description

While debugging a size regression in ng-bootstrap (ng-bootstrap/ng-bootstrap#3904 (comment)) when used in Angular 11 application I've spotted that a lot of dead code is retained because NG_VALUE_ACCESSOR is declared as a separate variable. Such pattern retains a linked component and consequently a lot of extra code used by this component. E.g. below code in the library will retain MylibComponent using Angular CLI 11, but will be removed as dead code using Angular CLI 10:

const TEST_VALUE_ACCESSOR = {
  provide: NG_VALUE_ACCESSOR,
  useExisting: forwardRef(() => MylibComponent),
  multi: true,
};

@Component({
  selector: 'lib-mylib',
  template: ` <p>mylib works!</p> `,
  styles: [],
  providers: [TEST_VALUE_ACCESSOR],
})
export class MylibComponent implements OnInit, ControlValueAccessor { ... }

Observations:

  • This only happens when code is present in the library. Component is eliminated as dead code if it is part of the application.
  • Inlining the variable will result in component being eliminated as dead code.
  • Library code processed by ngcc is identical, so I guess something in the application bundling has changed.

🔬 Minimal Reproduction

  1. Clone the reproductions for Angular 10 and 11 (fresh project + fresh lib + minimal code reproducing the problem, each in a separate commit):

    git clone -b va10 git@github.com:devoto13/va-regression-repro.git va10
    git clone -b va11 git@github.com:devoto13/va-regression-repro.git va11
    
  2. Build each repro:

    yarn ng build mylib --prod
    NG_BUILD_MANGLE=false ./node_modules/.bin/ng build --prod
    
  3. Check dist/va10/main.js and dist/va11/main.js respectively. Observe that in v10 project MylibComponent symbol is not present in the bundle, but in v11 project this symbol is retained in the bundle together with the value accessor.

🌍 Your Environment

Angular CLI: 11.0.3
Node: 14.8.0
OS: darwin x64

Angular: 11.0.3
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1100.3
@angular-devkit/build-angular   0.1100.3
@angular-devkit/core            11.0.3
@angular-devkit/schematics      11.0.3
@schematics/angular             11.0.3
@schematics/update              0.1100.3
ng-packagr                      11.0.3
rxjs                            6.6.3
typescript                      4.0.5

Anything else relevant?

@clydin
Copy link
Member

clydin commented Dec 8, 2020

The forwardRef(() => MylibComponent), aspect appears to be the source of the issue.
From a library source code perspective, there are two options to improve the optimization. The first would be to inline as suggested. This option would probably be the ideal scenario if the provider is not exported and only used in one component. This has the advantage of moving the provider definition into the component class and in turn reduces the complexity of statically analyzing and optimizing the code. The second option would be to add a pure annotation to the forwardRef call to signify that this module level call does not create any module level side effects.
From the Angular CLI perspective, there are also two options. The CLI can configure the optimizer to consider the forwardRef calls as pure so that they can be completely removed when the return value is unused. This however would not apply to users that are not using the Angular CLI. The second option would be to create additional library build-time passes that add relevant optimization hints to the library code prior to packaging. This second option would require a much larger effort but is something that is being considered in the future.

@devoto13
Copy link
Contributor Author

devoto13 commented Dec 8, 2020

@clydin Thanks for the detailed response. I've inlined all value accessors, but it still retains more code then it should. So I guess there is more to it. I'll continue the investigation tomorrow and post and update here once I have more information.

devoto13 added a commit to devoto13/ng-bootstrap that referenced this issue Dec 8, 2020
To fix dead code elimination using Angular 11.

See angular/angular-cli#19572 (comment) for more details. The *inline* option was chosen as ng-bootstrap does not needs to export these accessors and they're only used by a single component. This option appears the simplest to me, it is less likely to regress in the future and should work independently of whether consumer users Angular CLI or custom tooling to build their application. At the cost of slightly worse code readability.

Fixes ng-bootstrap#3904
devoto13 added a commit to devoto13/ng-bootstrap that referenced this issue Dec 8, 2020
To fix dead code elimination in Angular 11.

See angular/angular-cli#19572 (comment) for more details. The *inline* option was chosen as ng-bootstrap does not needs to export these providers and they're only used by a single component. This option appears the simplest to me, it is less likely to regress in the future and should work independently of whether consumer users Angular CLI or custom tooling to build their application. At the cost of slightly worse code readability.

Fixes ng-bootstrap#3904
@devoto13
Copy link
Contributor Author

devoto13 commented Dec 8, 2020

That was just one more forwardRef(), so never mind.

maxokorokov pushed a commit to ng-bootstrap/ng-bootstrap that referenced this issue Dec 22, 2020
To fix dead code elimination in Angular 11.

See angular/angular-cli#19572 (comment) for more details. The *inline* option was chosen as ng-bootstrap does not needs to export these providers and they're only used by a single component. This option appears the simplest to me, it is less likely to regress in the future and should work independently of whether consumer users Angular CLI or custom tooling to build their application. At the cost of slightly worse code readability.

Fixes #3904
santam85 pushed a commit to santam85/ng-bootstrap that referenced this issue Dec 28, 2020
To fix dead code elimination in Angular 11.

See angular/angular-cli#19572 (comment) for more details. The *inline* option was chosen as ng-bootstrap does not needs to export these providers and they're only used by a single component. This option appears the simplest to me, it is less likely to regress in the future and should work independently of whether consumer users Angular CLI or custom tooling to build their application. At the cost of slightly worse code readability.

Fixes ng-bootstrap#3904
@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 Jan 9, 2021
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 a pull request may close this issue.

2 participants