Skip to content

refactor: use light-weight token for optgroup to optimize bundle size#19577

Merged
devversion merged 3 commits intoangular:lightweight-token-optimizationsfrom
devversion:wip/light-weight-tokens-1
Jun 17, 2020
Merged

refactor: use light-weight token for optgroup to optimize bundle size#19577
devversion merged 3 commits intoangular:lightweight-token-optimizationsfrom
devversion:wip/light-weight-tokens-1

Conversation

@devversion
Copy link
Member

The MatOptgroup component is not always used in applications. e.g.
it's an optional part of the autocomplete or select. Currently though,
we always reference the MatOpgroup component directly at runtime,
and it will be retained in applications regardless of the usage.

We can improve this by providing a light-weight injection token for
the optgroup that we can then use in queries or optional DI injection.

This obviously was already an issue in View Engine too, but it became
significantly worse in Ivy as factories are now directly attached to
the component class.

Related to: #19576.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 9, 2020
@devversion devversion force-pushed the wip/light-weight-tokens-1 branch 3 times, most recently from 8de6705 to a62f1a5 Compare June 9, 2020 21:28
@devversion devversion marked this pull request as ready for review June 9, 2020 21:32
@devversion devversion requested a review from a team June 9, 2020 21:32
@devversion devversion added merge: preserve commits When the PR is merged, a rebase and merge should be performed target: patch This PR is targeted for the next patch release labels Jun 9, 2020
@devversion devversion force-pushed the wip/light-weight-tokens-1 branch from a62f1a5 to e63d9dd Compare June 12, 2020 08:24
@devversion devversion added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent needs rebase labels Jun 12, 2020
// TODO: Remove cast once https://github.com/angular/angular/pull/37506 is available.
/** @docs-private */
@ContentChildren(MatOptgroup, {descendants: true}) optionGroups: QueryList<MatOptgroup>;
@ContentChildren(MAT_OPTGROUP as any, {descendants: true}) optionGroups: QueryList<MatOptgroup>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether this would have an effect, considering that MatOptgroup has a hard reference in MatOptionModule which is then referenced in MatAutocompleteModule. Since we have the size checks set up, it should be easy to find out.

Copy link
Member Author

@devversion devversion Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double-check, but I was thinking "yes" (given the size in the test also decreased: e63d9dd). My thinking is that the module imports are just used for the compilation resolution, but the compiler will just reference the directives/components based on usage. Providers are persisted in the module injector definition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, double-checked. Everything works as expected. The module definitions by default will not reference declarations etc. (as noted above). The compiler will only reference these when emitInline is set. This is only the case in JIT when it gets through the compiler facade.

@devversion devversion force-pushed the wip/light-weight-tokens-1 branch from e63d9dd to 086efd1 Compare June 15, 2020 11:41
@devversion devversion added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent needs rebase labels Jun 15, 2020
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added lgtm action: merge The PR is ready for merge by the caretaker labels Jun 15, 2020
Copy link
Contributor

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devversion devversion force-pushed the wip/light-weight-tokens-1 branch from 086efd1 to c840d38 Compare June 16, 2020 19:23
The `MatOptgroup` component is not always used in applications. e.g.
it's an optional part of the autocomplete or select. Currently though,
we always reference the `MatOpgroup` component directly as a value,
and it will be retained in applications regardless of the usage.

We can improve this by providing a light-weight injection token for
the optgroup that we can then use in queries or optional DI injection.

This obviously was already an issue in View Engine too, but it became
significantly worse in Ivy as factories are now directly attached to
the component class.

Related to: angular#19576.
@devversion devversion changed the base branch from master to lightweight-token-optimizations June 17, 2020 17:13
@devversion devversion force-pushed the wip/light-weight-tokens-1 branch from c840d38 to d388020 Compare June 17, 2020 17:13
@devversion devversion merged commit edb8d17 into angular:lightweight-token-optimizations Jun 17, 2020
@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 Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: preserve commits When the PR is merged, a rebase and merge should be performed P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants