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

perf(router): cancel the navigation instantly if at least one resolver doesn't emit any value #45621

Closed

Conversation

dimakuba
Copy link
Contributor

Recently the navigation was on hold even at least one resolver didn't emit any value and completed, but another ones still are in progress to resolve any value. The changes cancel the navigation instantly if at least one resolver doesn't emit any value and completed.

@pullapprove pullapprove bot requested a review from atscott April 13, 2022 20:05
@dimakuba dimakuba force-pushed the cancel-navigation-instantly branch from 31b6768 to d47e1ba Compare April 14, 2022 11:13
@atscott
Copy link
Contributor

atscott commented Apr 14, 2022

Started a run of all internal tests against this change to verify it's not breaking.

@dimakuba in the meantime, can you update the symbol test goldens to get the CI test green?

@dimakuba
Copy link
Contributor Author

Started a run of all internal tests against this change to verify it's not breaking.

@dimakuba in the meantime, can you update the symbol test goldens to get the CI test green?

Yes, sure

@dimakuba dimakuba force-pushed the cancel-navigation-instantly branch 2 times, most recently from 37481d9 to 4eb18b0 Compare April 14, 2022 20:07
@atscott
Copy link
Contributor

atscott commented Apr 14, 2022

Green TGP

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

nit: can you update the commit message header to be perf(router):... so it's noted as a performance improvement? Refactor commits won't get added to the changelog so there won't be a note about this improvement anywhere!

@dylhunn dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: router target: major This PR is targeted for the next major release labels Apr 14, 2022
@ngbot ngbot bot modified the milestone: Backlog Apr 14, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Apr 14, 2022

@dimakuba Please @ me when you've addressed the above comment, and I will merge this PR.

…r doesn't emit any value

Recently the navigation was on hold even at least one resolver didn't emit any value and completed, but another ones still are in progress to resolve any value. The changes cancel the navigation instantly if at least one resolver doesn't emit any value and completed.
@dimakuba dimakuba force-pushed the cancel-navigation-instantly branch from 4eb18b0 to 8cffbd3 Compare April 15, 2022 08:17
@dimakuba dimakuba changed the title refactor(router): cancel the navigation instantly if at least one resolver doesn't emit any value perf(router): cancel the navigation instantly if at least one resolver doesn't emit any value Apr 15, 2022
@dimakuba
Copy link
Contributor Author

@dylhunn done

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 15, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Apr 15, 2022

This PR was merged into the repository by commit f13295f.

@dylhunn dylhunn closed this in f13295f Apr 15, 2022
@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 16, 2022
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: router target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants