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

feat(core): support APP_INITIALIZER work with observable #33222

Conversation

vthinkxie
Copy link
Contributor

@vthinkxie vthinkxie commented Oct 17, 2019

close #15088

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@vthinkxie vthinkxie requested a review from a team as a code owner October 17, 2019 09:50
@vthinkxie vthinkxie force-pushed the support_application_init_observerable branch 3 times, most recently from ac82f5b to fa6f57d Compare October 17, 2019 10:21
@vthinkxie vthinkxie requested a review from a team as a code owner October 17, 2019 10:21
@vthinkxie vthinkxie force-pushed the support_application_init_observerable branch 2 times, most recently from b77bf85 to 5253a23 Compare October 17, 2019 11:20
@matsko matsko added area: core Issues related to the framework runtime feature Issue that requests a new feature labels Oct 17, 2019
@ngbot ngbot bot modified the milestone: needsTriage Oct 17, 2019
@pullapprove pullapprove bot requested a review from alxhub July 27, 2020 20:15
@vthinkxie vthinkxie force-pushed the support_application_init_observerable branch 3 times, most recently from 92490df to da40ee2 Compare September 14, 2020 04:18
@vthinkxie
Copy link
Contributor Author

cc @alxhub

Copy link
Contributor

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

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

since toPromise is being deprecated, maybe we need to use something like await lastValueFrom?

@JiaLiPassion
Copy link
Contributor

We also need to update the doc here https://angular.io/guide/dependency-injection-providers

@josephperrott josephperrott removed request for a team November 3, 2020 00:31
@AndrewKushnir AndrewKushnir self-assigned this Feb 5, 2021
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Feb 5, 2021

Hi @vthinkxie, sorry we didn't have a chance to review your PR sooner.

As Jia mentioned above, the toPromise function is deprecated in RxJS and the lastValueFrom function should be used instead. Could you please update the code to use lastValueFrom function and also rebase this PR and resolve merge conflicts? Once the PR is updated we'll review the changes and provide additional feedback.

Thank you.

@vthinkxie vthinkxie force-pushed the support_application_init_observerable branch from da40ee2 to bb94e6b Compare February 5, 2021 07:33
@vthinkxie
Copy link
Contributor Author

vthinkxie commented Feb 5, 2021

Hi @vthinkxie, sorry we didn't have a chance to review your PR sooner.

As Jia mentioned above, the toPromise function is deprecated in RxJS and the lastValueFrom function should be used instead. Could you please update the code to use lastValueFrom function and also rebase this PR and resolve merge conflicts? Once the PR is updated we'll review the changes and provide additional feedback.

Thank you.

Hi @JiaLiPassion @AndrewKushnir
thanks a lot for your review, I have rebased the code onto master.
It seems that lastValueFrom is not available in rxjs 6 yet, maybe we can have another PR when rxjs 7 released?

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit cla: yes target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer cla: no labels Feb 19, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Feb 20, 2021
@AndrewKushnir
Copy link
Contributor

@vthinkxie just want to let you know that tests in Google's codebase went well, the PR was approved by all reviewers and it's ready for merge, so I'm adding it to the merge queue.

Thanks for implementing this feature and addressing all the feedback 👍

@vthinkxie
Copy link
Contributor Author

vthinkxie commented Feb 20, 2021

Hi @AndrewKushnir and @JoostK

thanks a lot for your review, looking forward to having this feature in the next angular version!

@loxy
Copy link

loxy commented Feb 20, 2021

Finally it came to an end! Nearly...
Now I know why it took so much time to include this feature to Angular. It was a pleasure to follow your work! Thank you all guys for supporting such a great tool!!! ❤️ Angular

@atscott atscott closed this in ca17ac5 Feb 22, 2021
atscott pushed a commit that referenced this pull request Feb 22, 2021
…e-effects (#33222)

Currently TestBed (both ViewEngine and Ivy) invoke `ApplicationInitStatus.runInitializers` as a part of the
bootstrap process to mimic real bootstrap steps. This is problematic for the `ApplicationInitStatus` class
tests since the `runInitializers` call performed by TestBed interfere with actual tests.

This commit updates ApplicationInitStatus tests to interact with the class directly instead of relying on TestBed
APIs to retrieve the class though DI.

PR Close #33222
@SandraVanDijk
Copy link

@vthinkxie Could you please tell me how to use this functionality?

I now have this:
provide: APP_INITIALIZER,
useFactory: (authService: AuthService) => () => { return authService.retrieveToken(); },
deps: [AuthService],
multi: true

I have angular version 12.0.0-next.5 installed, but it does not wait until the observable is resolved. The function authService.retrieveToken() does a http get call.

Hope you can give a bit more explanation. Thanks!

@AndrewKushnir
Copy link
Contributor

Hi @SandraVanDijk, could you please create a minimum repro using Stackblitz, so that we can have a look? Note: you could update package versions in the package.json file there (to 12.0.0-next.5) and the necessary packages would be installed. Once the repro is available, please create a new ticket via https://github.com/angular/angular/issues. Thank you.

@SandraVanDijk
Copy link

SandraVanDijk commented Mar 24, 2021

Hi @AndrewKushnir, thanks for your response. I tried to make a Stackblitz as requested, and I updated the packages.json as you told me. After I press save it did install packages and then it says "running ngcc", it doing that now for 2 hours. I never used this tool before, so not sure how to solve this. Is there maybe another way on how to share my code?

@AndrewKushnir
Copy link
Contributor

Is there maybe another way on how to share my code?

You could also try to create a new app (using ng new), update packages in package.json file, add relevant code needed for repro and put it on GitHub as a new repository.

@SandraVanDijk
Copy link

Hi @AndrewKushnir, thanks for that tip! While making a new repository I figured out what my mistake was. The function I was calling did not return an observable. So it made total sense APP_INILIALIZER did not wait.

I also found today this documentation: https://dzhavat.github.io/2021/02/25/using-observable-in-app-initializer.html, maybe this helps somebody else.

Thanks for your help!

@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 Apr 26, 2021
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 area: core Issues related to the framework runtime cla: yes cross-cutting: observables feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APP_INITIALIZER should also work with observables
8 participants