refactor(list): use light-weight tokens for injecting parent lists #19568
Conversation
googlebot
added
the
cla: yes
devversion
force-pushed
the
refactor/size-improvement-light-weight-tokens
branch
2 times, most recently
from
dc8e8eb to
f185ca3
Compare
devversion
added
P3
devversion
added
the
merge: preserve commits
integration/size-test/check-size.ts
Outdated
| const expectedSize = Number(golden[testId]); | ||
| const absoluteSizeDiff = Math.abs(actualSize - expectedSize); | ||
|
|
||
| // Whether the size deviates by 1% from the expected. |
There was a problem hiding this comment.
We should document this at the top of the file where it says if the file size changes "by certain amount", to express these same values.
Alternatively, maybe these constants should be at the top of the file for easier discovery.
There was a problem hiding this comment.
Rather than making this percentage based, what if it were just 1 kilobyte? Just because e.g. Overlay is big, doesn't mean we should necessarily tolerate larger size fluctuations in that package.
There was a problem hiding this comment.
@jelbourn Exactly, but that's why we have an absolute byte change check too. The percentage is helpful when a change is smaller than the absolute threshold but is considered large in percentage of the expected size.
This matches the same conditions as in angular/angular. The absolute threshold is currently 500 bytes.
There was a problem hiding this comment.
@josephperrott Moved it to the top-level as constants w/ comments. That should be better, agreed.
integration/size-test/check-size.ts
Outdated
| const expectedSize = Number(golden[testId]); | ||
| const absoluteSizeDiff = Math.abs(actualSize - expectedSize); | ||
|
|
||
| // Whether the size deviates by 1% from the expected. |
There was a problem hiding this comment.
Rather than making this percentage based, what if it were just 1 kilobyte? Just because e.g. Overlay is big, doesn't mean we should necessarily tolerate larger size fluctuations in that package.
Angular Material list items currently optionally inject a parent `MatNavList` or `MatList`. This has the downside of retaining these tokens at runtime because they are used for dependency injection as values. We can improve this by using so-called light-weight tokens. These allow us to continue injecting parent list or nav-lists, but without the risk of retaining the `MatList` and `MatNavList` classes with their factories. This was already the case before Angular v9 with View Engine, but the issue significance increases with Ivy where factories are now directly attached to the classes (such as `MatList` or `MatNavList`). Using light-weight tokens avoids this issue and gives us an additional size improvement. Notably this won't be an improvement when an application uses both the nav-list and standard `MatList`. Related to https://github.com/angular/angular-cli/issues/16866. More context on light-weight tokens in: https://hackmd.io/@mhevery/SyqDjUlrU#.
devversion
force-pushed
the
refactor/size-improvement-light-weight-tokens
branch
from
f185ca3 to
eb99a05
Compare
devversion
added
P2
andrewseguin
added
target: minor
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Angular Material list items currently optionally inject a parent
MatNavListor
MatList. This has the downside of retaining these tokens at runtimebecause they are used for dependency injection as values.
We can improve this by using so-called light-weight tokens. These allow
us to continue injecting parent list or nav-lists, but without the risk of
retaining the
MatListandMatNavListclasses with their factories.This was already the case before Angular v9 with View Engine, but
the problem became significantly worse with Ivy, where factories are now
directly attached to the classes (such as
MatListorMatNavList).Using light-weight tokens avoids this issue and gives us an additional
size improvement. Notably this won't be an improvement when an
application uses both the nav-list and standard
MatList.Related to angular/angular-cli#16866. More context on light-weight tokens in:
https://hackmd.io/@mhevery/SyqDjUlrU#.
Note: I'll be going through other components in the repo and send
similar changes (if applicable; and non-breaking).