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.

Sign up for GitHub

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

Jump to bottom

fix(http): wait for all XHR requests to finish before stabilizing application #49776

Closed
wants to merge 1 commit into from
Closed

fix(http): wait for all XHR requests to finish before stabilizing application #49776

wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Apr 11, 2023 •
edited

Previously, since the HttpXhrBackend is a singleton, the macrotask was created and completed only for the initial request since it was stored as in property in the class instance. This commit replaces this logic to create a macro task for every XHR request.

Closes #49730

@alan-agius4 alan-agius4 added area: core Issues related to the framework runtime area: common/http labels Apr 11, 2023
@ngbot ngbot bot modified the milestone: Backlog Apr 11, 2023
@alan-agius4 alan-agius4 added target: rc This PR is targeted for the next release-candidate action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 11, 2023
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Apr 11, 2023 •
edited

For visibility, I did try to use InitialRenderPendingTasks 16555a4 but this caused fakeAsync tests to fail due to the pending timer. The timer was required due as the InitialRenderPendingTasks is promised based and completes when there are no pending tasks and there is no way to go back to a non complete state.

@alan-agius4 alan-agius4 force-pushed the http-pending-task branch 4 times, most recently from 4e12fba to 1aa60ff Compare April 11, 2023 15:02
@AndrewKushnir
Copy link
Contributor

Presubmit.

AndrewKushnir
AndrewKushnir approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the fix Alan 👍

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 11, 2023
@ngbot
Copy link

ngbot bot commented Apr 11, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    pending status "pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir
AndrewKushnir approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a 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

jessicajaniuk
jessicajaniuk approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a 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

@alan-agius4
fix(http): wait for all XHR requests to finish before stabilizing app…
75acf24 
…lication

Previously, since the `HttpXhrBackend` is a singleton, the macrotask was created and completed only for the initial request since it was stored as in property in the class instance. This commit replaces this logic to create a macro task for every XHR request.

Closes angular#49730
@alan-agius4 alan-agius4 changed the title fix(http): wait for all XHR requires to finish before stabilizing application fix(http): wait for all XHR requests to finish before stabilizing application Apr 11, 2023
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 079f4bc.

AndrewKushnir pushed a commit that referenced this pull request Apr 11, 2023
@alan-agius4 @AndrewKushnir
fix(http): wait for all XHR requests to finish before stabilizing app…
2eb9b8b 
…lication (#49776)

Previously, since the `HttpXhrBackend` is a singleton, the macrotask was created and completed only for the initial request since it was stored as in property in the class instance. This commit replaces this logic to create a macro task for every XHR request.

Closes #49730

PR Close #49776
@AndrewKushnir AndrewKushnir mentioned this pull request Apr 11, 2023
Closed
@alan-agius4 alan-agius4 deleted the http-pending-task branch April 11, 2023 20:50
@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 May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common/http area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

ApplicationRef.isStable doesn't wait for all requests finished on SSR with angular 16.
3 participants
@alan-agius4 @AndrewKushnir @jessicajaniuk