Skip to content
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

fix(core): run `APP_INITIALIZER`s before accessing `LOCALE_ID` token in Ivy TestBed #36237

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Mar 25, 2020

Prior to this commit, Ivy TestBed was accessing locale ID before APP_INITIALIZER functions were called. This execution order is not consistent with the app bootstrap logic in the application_ref.ts file. This commit updates Ivy TestBed execution order to call initializers first (since they might affect LOCALE_ID token value) and accessing and setting locale ID after that.

Related changes were performed in the application_ref.ts file in PR #34830.

Fixes #36230.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
…in Ivy TestBed

Prior to this commit, Ivy TestBed was accessing locale ID before `APP_INITIALIZER` functions were called. This execution order is not consistent with the app bootstrap logic in `application_ref.ts`. This commit updates Ivy TestBed execution order to call initializers first (since they might affect `LOCALE_ID` token value) and accessing and setting locale ID after that.

Fixes #36230.
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Mar 25, 2020

@AndrewKushnir AndrewKushnir marked this pull request as ready for review Mar 25, 2020
Copy link
Member

petebacondarwin left a comment

This looks good for synchronous initialisers. So this is better than the current situation. Therefore I'm happy to land this for now.

But it is possible for initialisers to be asynchronous by returning a Promise. It looks to me like the TestBed will currently only work with synchronous initialisers since there is no code to wait for the ApplicationInitStatus to be resolved. The corresponding code in the PlatformRef class waits for the status object to resolve the donePromise before setting the locale id. See

const initStatus: ApplicationInitStatus = moduleRef.injector.get(ApplicationInitStatus);
initStatus.runInitializers();
return initStatus.donePromise.then(() => {
if (ivyEnabled) {
// If the `LOCALE_ID` provider is defined at bootstrap then we set the value for ivy
const localeId = moduleRef.injector.get(LOCALE_ID, DEFAULT_LOCALE_ID);
setLocaleId(localeId || DEFAULT_LOCALE_ID);
}
this._moduleDoBootstrap(moduleRef);
return moduleRef;
});

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Mar 26, 2020

Thanks for review @petebacondarwin.

You are right, TestBed doesn't work with async app initializers in Ivy and View Engine (see test_bed.ts for View Engine version):

(this._moduleRef.injector.get(ApplicationInitStatus) as any).runInitializers();

Implementing this support may require extending TestBed API and it's outside of the scope of this PR (since the current logic is consistent in Ivy and VE).

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Mar 26, 2020

FYI, Global and Blueprint presubmits are successful.

alxhub added a commit that referenced this pull request Mar 27, 2020
…in Ivy TestBed (#36237)

Prior to this commit, Ivy TestBed was accessing locale ID before `APP_INITIALIZER` functions were called. This execution order is not consistent with the app bootstrap logic in `application_ref.ts`. This commit updates Ivy TestBed execution order to call initializers first (since they might affect `LOCALE_ID` token value) and accessing and setting locale ID after that.

Fixes #36230.

PR Close #36237
@alxhub alxhub closed this in 1649743 Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.