Skip to content

refactor(form-field): use lightweight token for optimized treeshaking #19714

Merged
devversion merged 2 commits intoangular:lightweight-token-optimizationsfrom
devversion:wip/light-weight-tokens-11
Jun 22, 2020
Merged

refactor(form-field): use lightweight token for optimized treeshaking #19714
devversion merged 2 commits intoangular:lightweight-token-optimizationsfrom
devversion:wip/light-weight-tokens-11

Conversation

@devversion
Copy link
Member

@devversion devversion commented Jun 22, 2020

Currently the MatFormField always retains MatHint, MatError,
MatPrefix and MatSuffix. These are optional parts of a form-field so
we ideally would not retain those always.

We can achieve this by using lightweight injection tokens for querying
of these directives, instead of using the actual implementations as
query predicate token. The retention was already an issue in View Engine
too, but the issue became more noticeable in Ivy where factories and
definitions are directly attached to the classes.

In the basic case this signifies a reduction of ~800kb, while in the advanced
case (due to the referenced injection tokens), we increase by ~300kb.

Related to #19576

Currently the `MatFormField` always retains `MatHint`, `MatError`,
`MatPrefix` and `MatSuffix`. These are optional parts of a form-field so
we ideally would not retain those always.

We can achieve this by using lightweight injection tokens for querying
of these directives, instead of using the actual implementations as
query predicate token. The retention was already an issue in View Engine
too, but the issue became more noticable in Ivy where factories and
definitions are directly attached to the classes.
@devversion devversion added P2 The issue is important to a large percentage of users, with a workaround merge safe labels Jun 22, 2020
@devversion devversion requested a review from a team June 22, 2020 13:36
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 22, 2020
@@ -1,3 +1,5 @@
export declare const _MAT_HINT: InjectionToken<MatHint>;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this one underscored, but the others aren't?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be a JSDoc for the _MAT_HINT constant. In short: The MDC-based form-field uses MatHint directly, so it's never optional. Hence a lightweight token does not make any sense to have, and to avoid a breaking change when eventually the MDC-based form-field is used, the lightweight token for MatHint in non-MDC is private.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to just keep it consistent in that case and not use the token? It's not that large to begin with.

Copy link
Member Author

@devversion devversion Jun 22, 2020

Choose a reason for hiding this comment

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

All of the classes are not really large in source here, but with the generated factory and definition they could easily sum-up (even more in ES5). IIRC for hint it's about 230kb.

I don't think that this should stop us from having a lightweight token in the non-MDC form-field. It's a non-public facing implementation detail, and the form-fields deviate quite a bit anyway already.

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 the lgtm label Jun 22, 2020
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

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

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jun 22, 2020
@devversion devversion merged commit 6447eb3 into angular:lightweight-token-optimizations Jun 22, 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 23, 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 P2 The issue is important to a large percentage of users, with a workaround

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants