feat(router) ✨ Adds Asynchrounous Redirects #60863
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@MeddahAbdellah Thanks for the contribution! Looks like you have some failing tests and lint issues to fix. |
4611713 to
6b0b229
Compare
|
Apologies, forgot to put the PR in draft. I have updated the typo that caused the tests to fail, I'll launch them again and put the PR to review once everything is green |
wartab
reviewed
Apr 14, 2025
atscott
reviewed
Apr 14, 2025
658dd30 to
8fa60d4
Compare
8fa60d4 to
35a7b08
Compare
35a7b08 to
c289ae9
Compare
atscott
approved these changes
Apr 22, 2025
atscott
approved these changes
Apr 22, 2025
thePunderWoman
approved these changes
Apr 22, 2025
8000
|
Note to self: This should likely be marked as a breaking change |
c289ae9 to
ba2907b
Compare
Adds support for asynchronous redirects in the router, allowing redirect logic to be resolved dynamically (e.g., via API or async function). This enhances routing flexibility and supports more complex navigation scenarios. BREAKING CHANGE: The `RedirectFn` can now return `Observable` or `Promise`. Any code that directly calls functions returning this type may need to be adjusted to account for this.
ba2907b to
5c7d909
Compare
MeddahAbdellah
commented
Apr 22, 2025
Comment on lines
+200
to
+214
| function getRedirectResult( | ||
| redirectTo: string | RedirectFunction, | ||
| currentSnapshot: ActivatedRouteSnapshot, | ||
| injector: Injector, | ||
| ): Observable<string | UrlTree> { | ||
| if (typeof redirectTo === 'string') { | ||
| return of(redirectTo); | ||
| } | ||
| const redirectToFn = redirectTo; | ||
| const {queryParams, fragment, routeConfig, url, outlet, params, data, title} = currentSnapshot; | ||
| return wrapIntoObservable( | ||
| runInInjectionContext(injector, () => | ||
| redirectToFn({params, data, queryParams, fragment, routeConfig, url, outlet, title}), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Thought (can be ignored): I don't know if it's better to pull it into a new function. I have had a hard time going up the stack trace when trying to debug. granted this is more elegant, I have been finding myself avoiding too much depth in my function calls (I mean by that, a function that calls a function that calls a function), it's easier to debug I find but that remains a personal opinion.
|
This PR was merged into the repository by commit 62de7d9. The changes were merged into the following branches: main |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 sin
2ED5
gle 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.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently the angular router only allows a string or a synchronous function for the redirectTo field of the router.
What is the new behavior?
Currently the angular router now allows also asynchronous functions that return a Promise or an Observable.
Does this PR introduce a breaking change?
Other information
I had to change one of the tests whose assertion was unreachable for it to fail. There might be a bug in the redirection strategy. I don't know if it's done on purpose, but if the path that has the redirectTo is a subpath of the redirected to url. The redirectTo function returns it as a UrlTree, which causes an infinite loop. I have been able to reproduce it here:
https://stackblitz.com/edit/angular-vakejq-tmlfa9gy?file=src%2Fapp.routes.ts
I have added a test to lock this behaviour, but I suspect it should be modified, at least throwing an error in devMode telling the user that there might be an infinite loop. I'd gladly do it in another PR once a decision on what should be done is made. In the meantime, this PR adds async management of redirection, keeping all the current behaviours of the simple functional redirection. All tests pass locally.