GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat(router): Add ability to create `UrlTree` from any `ActivatedRouteSnapshot` by atscott · Pull Request #45877 · angular/angular

Conversation

@atscott

@atscott atscott commented May 4, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This exposes a new function from the router public API that allows
developers to create a UrlTree from any ActivatedRouteSnapshot.
The current Router APIs only support creating a UrlTree from an
ActivatedRoute which is both active and materially appears in the
UrlTree (it cannot be an empty path named outlet). This is because the
implementation of the current way of creating a UrlTree attempts to
look up the UrlSegment of the ActivatedRoute in the currently active
UrlTree of the router. When this doesn't work, the UrlTree creation
fails.

Note that this API does not replace the current one. That would actually be a
breaking change but should be done at some point in the future (v15). That is,
router.navigate should call this new function. At that point, we can
remove _lastPathIndex, _urlSegment, and _correctedPathIndex from
the ActivatedRoute, along with all of the logic associated with
determining what those should be. In addition, this would unblock a fix
for #26081 because the applyRedirects and recognize operations
could be combined into one. Overall, this would simplify logic in the router
and reduce code size. It also exposes core routing capabilities as a helper function
which were previously private API, which is a necessary step towards #42953.

As a stress test for this new function, it was swapped in as the
default for UrlTree creation in #45859 and tested internally. The
results indicate that this function behaves correctly.

resolves #42191 (Tested directly)
resolves #38276 (The test with a guard covers this case)
resolves #22763 (Tested directly)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

stLmpp and Platonn reacted with thumbs up emoji Totati and zakhenry reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 2 reactions
@atscott atscott added state: WIP area: router labels May 4, 2022
@atscott atscott added this to the v14.1-candidates milestone May 4, 2022
@atscott atscott changed the title feat(router): Add ability to create UrlTree from any `ActivatedRout… feat(router): Add ability to create UrlTree from any ActivatedRouteSnapshot May 4, 2022
@atscott
atscott force-pushed the createUrlTreeFromSnapshot branch from 4e3cc60 to afc95ad Compare May 4, 2022 21:43
@atscott atscott mentioned this pull request May 4, 2022
Rearchitect Router so it's more modular #42953
Closed
@atscott
atscott force-pushed the createUrlTreeFromSnapshot branch from afc95ad to 39baf5c Compare May 4, 2022 22:05
@alfaproject

alfaproject commented May 5, 2022

Copy link
Copy Markdown
Contributor

Shouldn't we deprecate the 'old way' right away, so that it can be eventually deleted sooner rather than later?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@atscott

atscott commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

@alfaproject The "old way" will be replaced with this new function as a breaking change rather than deprecating it. That is, the createUrlTree call in router.createUrlTree will just be replaced. I did deprecate relativeLinkResolution: 'legacy' in v14 because it was really difficult to match that buggy behavior with this simpler API (I actually couldn't even fully figure out how to).

alfaproject reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@atscott
atscott force-pushed the createUrlTreeFromSnapshot branch from 39baf5c to 62b9065 Compare May 17, 2022 17:19
@atscott
feat(router): Add ability to create UrlTree from any `ActivatedRout…
37403d9
…eSnapshot`

This exposes a new function from the router public API that allows
developers to create a `UrlTree` from _any_ `ActivatedRouteSnapshot`.
The current Router APIs only support creating a `UrlTree` from an
`ActivatedRoute` which is both active _and_ materially appears in the
`UrlTree` (it cannot be an empty path named outlet). This is because the
implementation of the current way of creating a `UrlTree` attempts to
look up the `UrlSegment` of the `ActivatedRoute` in the currently active
`UrlTree` of the router. When this doesn't work, the `UrlTree` creation
fails.

Note that this API does not replace the current one. That would actually be a
breaking change but should be done at some point in the future (v15). That is,
`router.navigate` should call this new function. At that point, we can
remove `_lastPathIndex`, `_urlSegment`, and `_correctedPathIndex` from
the `ActivatedRoute`, along with all of the logic associated with
determining what those should be. In addition, this would unblock a fix
for angular#26081 because the `applyRedirects` and `recognize` operations
could be combined into one.  Overall, this would simplify logic in the router
and reduce code size. It also exposes core routing capabilities as a helper function
which were previously private API, which is a necessary step towards angular#42953.

As a stress test for this new function, it _was_ swapped in as the
default for `UrlTree` creation in angular#45859 and tested internally. The
results indicate that this function behaves correctly.

resolves angular#42191 (Tested directly)
resolves angular#38276 (The test with a guard covers this case)
resolves angular#22763 (Tested directly)
@atscott
atscott force-pushed the createUrlTreeFromSnapshot branch from 62b9065 to 37403d9 Compare May 17, 2022 17:22
@atscott

atscott commented May 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

presubmit - Also triggered TGP for this since there is a small change to existing code.

Edit: Green TGP

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@atscott
atscott marked this pull request as ready for review May 18, 2022 16:20
@atscott atscott added target: minor This PR is targeted for the next minor release and removed state: WIP labels May 18, 2022
@pullapprove
pullapprove Bot requested review from AndrewKushnir, jessicajaniuk and pkozlowski-opensource May 18, 2022 16:20
AndrewKushnir
AndrewKushnir reviewed May 18, 2022
View reviewed changes
Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pullapprove
pullapprove Bot requested a review from dylhunn May 18, 2022 23:10
@atscott
fixup! feat(router): Add ability to create UrlTree from any `Activa…
d719254
…tedRouteSnapshot`
AndrewKushnir
AndrewKushnir reviewed May 19, 2022
View reviewed changes
Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@atscott
fixup! feat(router): Add ability to create UrlTree from any `Activa…
cbf1914
…tedRouteSnapshot`
AndrewKushnir
AndrewKushnir approved these changes May 20, 2022
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just for additional context: we discussed the commands argument question offline. The summary is that it's a well established term in Router APIs and this API has symmetry with other APIs. The changes looks great 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/router/src/create_url_tree.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

AndrewKushnir
AndrewKushnir approved these changes May 20, 2022
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jessicajaniuk
jessicajaniuk approved these changes May 23, 2022
View reviewed changes

@jessicajaniuk jessicajaniuk left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jessicajaniuk
jessicajaniuk removed request for dylhunn and pkozlowski-opensource May 23, 2022 16:46
@jessicajaniuk jessicajaniuk added the action: merge The PR is ready for merge by the caretaker label May 23, 2022
@atscott
fixup! feat(router): Add ability to create UrlTree from any `Activa…
2c14317
…tedRouteSnapshot`
@atscott atscott added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium labels May 23, 2022
@atscott

atscott commented May 23, 2022

Copy link
Copy Markdown
Contributor Author

caretaker note: Most of the changes are part of the new API and only minimally touches existing router code. For a router change, this is comparatively low risk but if you could merge and sync on its own or other low risk PRs, that would be a good idea.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@alxhub

alxhub commented May 24, 2022

Copy link
Copy Markdown
Member

This PR was merged into the repository by commit 53ca936.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@alxhub alxhub closed this in 53ca936 May 24, 2022
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jun 24, 2022

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jun 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes
+1 more reviewer
@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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 risk: medium target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

v14.1-candidates

Development

Successfully merging this pull request may close these issues.

Paths generated for child outlets relative to empty path named outlet parent are incorrect Router.navigate doesnt work with relative route unless that route is currently active bug: relative navigation doesn't work inside route guards

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@atscott @alfaproject @alxhub @AndrewKushnir @jessicajaniuk
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.