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
perf(platform-browser): avoid including Testability by default in bootstrapApplication
#45885
perf(platform-browser): avoid including Testability by default in bootstrapApplication
#45885
Conversation
efde95e
to
f2b0f6d
Compare
Note: CI indicates ~3.5kb of payload size savings for minified version (but before gzip):
|
5683a51
to
a3ce7e2
Compare
a3ce7e2
to
f1a7cfe
Compare
Update: I've performed manual testing with the following e2e tools (that are presented as options in
All of them work correctly with the For Protractor users, this PR adds the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: fw-core, integration-tests, public-api, size-tracking
@@ -235,6 +235,9 @@ export class TransferState { | |||
// @public (undocumented) | |||
export const VERSION: Version; | |||
|
|||
// @public | |||
export function withProtractorTestingSupport(): Provider[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of iffy on this function starting with with
, since I've usually only seen this used with builder-style APIs.
Why not just protractorTestingSupport
? That way its use in providers reads like "provide protractor testing support"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @jelbourn 👍
The function will only be used (or rather makes sense) in a context of bootstrapping an app and the call site would look like this:
const appRef = bootstrapApplication(
RootStandaloneComponent,
{providers: [withProtractorTestingSupport()]}
);
So we bootstrap an app with protractor testing support
- this is where the with
prefix comes from. We plan to use similar prefixes for other things (like withRouter
, etc). Please let us know what do you think.
// cc @alxhub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I looked at the usage in the PR. I think that this is inconsistent with how providers like this are normally named and that it's not clear what with
is modifying. It would be different if it were e.g.
bootstrapApplication(RootStandaloneComponent, providers: [...])
.withProtractorSupport();
In that scenario, the with
makes sense because it's clearly modifying the bootstrap call. (This builder-style API of course defeats the point of tree-shaking). But providers are traditionally more "noun-y". If you mix this API with other providers, the mismatch stands out:
const appRef = bootstrapApplication(
RootStandaloneComponent,
{providers: [
UserDataClient,
SideChannelDispatcher,
withProtractorTestingSupport()
]}
);
I read this code like:
"Provide a user data client"
"Provide a side channel dispatcher"
"Provider a with protractor testing support"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some additional discussions, the function was renamed to provideProtractorTestingSupport
. We'll have another round of discussions and finalize the name during the RC phase (before v14 final release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: size-tracking
Withholding public API review so as to not cut short the ongoing conversation about the naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: public-api, size-tracking
But as atscott I don't intend to cut off the conversation above.
* which requires Testability. Testability is not included by default when | ||
* the `bootstrapApplication` function is used (which is the case in this app). | ||
* We use this app primarily to measure payload size, so we want to keep | ||
* Testability excluded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have this e2e test / e2e testing infrastructure at all then? Or even this integration/standalone-bootstrap
integration test at all? It seems like we are trying to have 2 things covered by this "app":
- size tracking;
- actual integration testing of the bootstrap operation.
It seems like those 2 needs are in conflict somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkozlowski-opensource currently those two parts (size tracking and e2e tests) are bundled together in one build rule:
angular/integration/standalone-bootstrap/BUILD.bazel
Lines 3 to 7 in 74321a4
ng_integration_test( | |
name = "test", | |
setup_chromium = True, | |
track_payload_size = "standalone-bootstrap", | |
) |
Ideally we'd need to decouple them into 2 separate build rules, so that we have more flexibility in terms of configuration. Once it's done, we can refactor the usage here to just leverage the size-tracking piece.
…otstrapApplication` The Testability-related logic was refactored in angular#45657 to become tree-shaking-friendly: it was decoupled from the core providers of the `BrowserModule`. This commit updates the newly-introduced `bootstrapApplication` function to exclude Testability-providers by default (note: the Testability is still included in the NgModule-based bootstrap). In order to add the Testability to the app bootstrapped via `bootstrapApplication`, the `provideProtractorTestingSupport` function is introduced.
07a6eb4
f1a7cfe
to
07a6eb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed-for: public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed-for: public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: fw-core, integration-tests, public-api, size-tracking
This PR was merged into the repository by commit 3165fa3. |
…otstrapApplication` (#45885) The Testability-related logic was refactored in #45657 to become tree-shaking-friendly: it was decoupled from the core providers of the `BrowserModule`. This commit updates the newly-introduced `bootstrapApplication` function to exclude Testability-providers by default (note: the Testability is still included in the NgModule-based bootstrap). In order to add the Testability to the app bootstrapped via `bootstrapApplication`, the `provideProtractorTestingSupport` function is introduced. PR Close #45885
After angular/angular#45885, testability now needs to be explicitly added to `bootstrapApplication()`.
After angular/angular#45885, testability now needs to be explicitly added to `bootstrapApplication()`.
After angular/angular#45885, testability now needs to be explicitly added to `bootstrapApplication()`. (cherry picked from commit 329a2a3)
After angular/angular#45885, testability now needs to be explicitly added to `bootstrapApplication()`. (cherry picked from commit 329a2a3)
@AndrewKushnir Thanks for your response. Thanks. |
@StanislavKharchenko the If we end up deprecating the
Could you please share a bit more on how you use |
@AndrewKushnir We use FrameworkStabilizer to wait until Angular application finish rendering and available for interaction.
We need some condition to check that all pending http requests and angular rendering finished so it guarantee that elements on page will not change. Is there any alternative API provided by Angular to execute js code in browser which say us that Angular fully rendered? |
@StanislavKharchenko thanks for additional information.
As a possible option you can use
or directly from the
Similar approach is used for the SSR use-cases (see here). Please let me know if that would work for your use-case. Thank you. |
@AndrewKushnir It seems this solution is suitable in case if e2e tests are under application structure. Could "ApplicationRef" be accessible from browser not in dev. mode? |
@StanislavKharchenko I think you can try exposing the
|
@AndrewKushnir So it looks like relevant with v14 release when "bootstrapApplication" will be available. |
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. |
The Testability-related logic was refactored in #45657 to become tree-shaking-friendly: it was decoupled from the core providers of the
BrowserModule
. This commit updates the newly-introducedbootstrapApplication
function to exclude Testability-providers by default (note: the Testability is still included in the NgModule-based bootstrap).In order to add the Testability to the app bootstrapped via
bootstrapApplication
, thewithProtractorTestingSupport
function is introduced.PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
bootstrapApplication
function is not released yet (will be released in v14).