feat(benchmarks): setup for benchmarking components #19320
Conversation
googlebot
added
the
cla: yes
wagnermaciel
force-pushed
the
feat/benchpress
branch
from
385a259 to
0f4a882
Compare
wagnermaciel
force-pushed
the
feat/benchpress
branch
2 times, most recently
from
8bd51e5 to
62182c2
Compare
wagnermaciel
force-pushed
the
feat/benchpress
branch
4 times, most recently
from
6504d50 to
1fd3d2c
Compare
wagnermaciel
force-pushed
the
feat/benchpress
branch
2 times, most recently
from
ddddc80 to
0f41f98
Compare
| </ng-container> | ||
| `, | ||
| encapsulation: ViewEncapsulation.None, | ||
| styleUrls: ['//src/material/core/theming/prebuilt/indigo-pink.css'], |
There was a problem hiding this comment.
Did you confirm if that actually works? I wouldn't know of any resolution code that supports Bazel labels.
There was a problem hiding this comment.
@devversion Yeah, it works. Removing it breaks the styles. Kinda strange now that you mention it... @jelbourn Does styleUrls supports Bazel labels? lol
There was a problem hiding this comment.
I have no idea how this would work. @devversion and @wagnermaciel could you pair to figure out the best way to set this up?
There was a problem hiding this comment.
Did you think about the impact of using styleUrls for that? couldn't it affect benchmarks in creation phase as the theme styles are actually loading deferred as part of the runtime JS while we'd be actually only interested in the test component in isolation?
There was a problem hiding this comment.
Just checked. It works only with ngtsc (so not with View Engine) because ngtsc normalizes the path so that double-slash (as per Bazel label) becomes a path. That path is then matched against rootDirs of the TS program. bazel-out is a root directory, so the prebuilt theme can be resolved. That is reasonable (with the downside of not working for --config=view-engine).
Regardless of that though, I think we should not load the theme as part of the component factory. That will cause deviations in benchmarks as the theme will be loaded deferred and through Angular. I still think we should implement asset injection.
I think we could merge this in the interim. Asset injection would require upstream changes in dev-infra. @wagnermaciel Happy to jump on a call to explain what I refer to with asset injection
There was a problem hiding this comment.
That explains it! Yeah, I agree that we can leave this for now and change it to an asset in a follow-up. @wagnermaciel can you add a TODO and file an issue for yourself?
| </ng-container> | ||
| `, | ||
| encapsulation: ViewEncapsulation.None, | ||
| styleUrls: ['//src/material/core/theming/prebuilt/indigo-pink.css'], |
There was a problem hiding this comment.
I have no idea how this would work. @devversion and @wagnermaciel could you pair to figure out the best way to set this up?
jelbourn
added
lgtm
action: merge
* Set up components repo to use dev-infra benchmarking tools * Created first set of benchmark tests for mat-checkbox * Import indigo-pink.scss via the components styleUrls * Set the view encapsulation of the component to none so that the styles from indigo-pink.scss are applied to the whole page * Left a todo in WORKSPACE to remember to deduplicate browsers * Left a todo in the checkbox benchmark's BUILD file to remember to update the component_benchmark once the desired changes are made
wagnermaciel
force-pushed
the
feat/benchpress
branch
from
51996b0 to
856f496
Compare
wagnermaciel
removed
the
merge: fix commit message
jelbourn
added
target: major
|
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. |
Summary
Current Workaround
This change depends on two changes to angular/angular.
Until those changes have been made, the current workaround is to
yarn bazel build //dev-infra:npm_package.packto create a local version of dev-infraThis workaround allows us to change the package.json in angular/components to depend on the files from this locally published version of dev-infra.