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

Combine recognize and apply redirects #49163

Closed
wants to merge 2 commits into from
Closed

Combine recognize and apply redirects #49163

wants to merge 2 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Feb 22, 2023

No description provided.

@ngbot ngbot bot modified the milestone: Backlog Feb 22, 2023
@atscott atscott force-pushed the combineRecognizeAndApplyRedirects branch 3 times, most recently from 9f3d0c4 to e53bc2a Compare February 22, 2023 22:04
@atscott atscott marked this pull request as ready for review February 27, 2023 22:08
AndrewKushnir
AndrewKushnir reviewed Feb 28, 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.

@atscott I've looked through the change and I'd like to ask if we can pair to view it together, so I can better understand the context. Thank you.

goldens/size-tracking/integration-payloads.json Outdated Show resolved Hide resolved
packages/router/src/recognize.ts Outdated Show resolved Hide resolved
@atscott atscott force-pushed the combineRecognizeAndApplyRedirects branch 2 times, most recently from 4ba3d6d to 627d211 Compare March 1, 2023 17:03
@atscott atscott force-pushed the combineRecognizeAndApplyRedirects branch from 627d211 to f49bbd3 Compare March 27, 2023 21:33
AndrewKushnir
AndrewKushnir approved these changes Mar 27, 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.

👍

AndrewKushnir
AndrewKushnir approved these changes Mar 27, 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: size-tracking

@atscott atscott removed the request for review from dylhunn March 27, 2023 22:46
@atscott atscott added action: global presubmit The PR is in need of a google3 global presubmit action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: global presubmit The PR is in need of a google3 global presubmit labels Mar 27, 2023
@atscott
Copy link
Contributor Author

atscott commented Mar 28, 2023

Green TGP: https://fusion2.corp.google.com/presubmit/tap/519850236/OCL:519850236:BASE:520045700:1680019909910:3411ac52/targets

@atscott
Copy link
Contributor Author

atscott commented Mar 28, 2023

This PR was merged into the repository by commit 1600687.

@atscott atscott closed this in 1600687 Mar 28, 2023
atscott added a commit to atscott/angular that referenced this pull request Mar 29, 2023
@atscott
Revert "fix(router): Route matching should only happen once when navi…
fa35f85 
…gating (angular#49163)"

This reverts commit 1600687.
@atscott atscott mentioned this pull request Mar 29, 2023
Closed
atscott added a commit that referenced this pull request Mar 29, 2023
@atscott
Revert "fix(router): Route matching should only happen once when navi…
0432d95 
…gating (#49163)" (#49634)

This reverts commit 1600687.

PR Close #49634
@atscott atscott reopened this Mar 29, 2023
@atscott
fix(router): Route matching should only happen once when navigating
836e434 
When navigating in the Router, the current approach does the redirects
and the creation of the `RouterStateSnapshot` in two separate steps
(applyRedirects and recognize). These two steps duplicate the route
matching logic, resulting in user code on routes being executing twice
(custom `UrlMatcher` and `canMatch` guards). This also duplicates the
complex matching logic in two places, which increases the bundle size
and maintenance burden.

This commit combines the `applyRedirects` and `recognize` steps into a
single matching algorithm.

fixes angular#26081
@atscott
fixup! fix(router): Route matching should only happen once when navig…
ea1a12d 
…ating
@atscott atscott force-pushed the combineRecognizeAndApplyRedirects branch from f49bbd3 to ea1a12d Compare March 29, 2023 20:45
@atscott
Copy link
Contributor Author

atscott commented Mar 29, 2023

Re-merging. I've confirmed this was an issue with the internal route configuration.

@atscott
Copy link
Contributor Author

atscott commented Mar 29, 2023

This PR was merged into the repository by commit 3c7e637.

@atscott atscott closed this in 3c7e637 Mar 29, 2023
@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 29, 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

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note router: config matching/activation/validation target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@atscott @AndrewKushnir