feat(router) added method to convert an Url to RouterStateSnapshot by smurfy · Pull Request #15826 · angular/angular

smurfy · 2017-04-07T10:10:20Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

There is no clean way to convert an url to a route.
See #15750

What is the new behavior?

You now can convert any url to a routestatesnapshot (if they exist).
This is will allows you to easily generate breadcrumbs or create a directive to check access to a routeLink

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

Other information:

I did not include any tests because of the size of the change and the actual logic is in the recognize function which is already has its own tests.

googlebot · 2017-04-07T10:10:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

smurfy · 2017-04-07T10:12:40Z

I signed it!

googlebot · 2017-04-07T10:12:43Z

CLAs look good, thanks!

bijanajamlou · 2017-05-05T07:14:36Z

What is the status of this? When can we expect this feature to be released?

tartexs · 2017-06-06T12:13:52Z

what's the status of this? any advances? I really need something like this

jasonaden

Thanks for the PR and sorry for the delay. This looks very useful.

One change requested, and please add a test for this method.

jasonaden · 2017-07-13T23:06:00Z

packages/router/src/router.ts

Our Observables do not have the Rxjs prototype methods. So you would need to import toPromise in order to return a promise here. So:

import {toPromise} from 'rxjs/operator/toPromise'; ... return toPromise.call(snapshot$);

jasonaden · 2017-07-26T17:29:02Z

@smurfy Are you able to make the updates on this PR?

smurfy · 2017-07-26T20:53:22Z

@jasonaden I will have a look tomorrow regarding the promise thing.

Regarding Unit tests i need to read-up on. We only do selenium tests and as mentioned in the initial comment, the new method is only a wrapper for "recognize" which is already unit tested quite heavily. So i'm not sure if we really need unit tests.

smurfy · 2017-07-27T16:24:00Z

I updated my PR with your suggested changes and added an unit-test.
I also did a test-build and everything still works with 4.3.1

smurfy · 2017-08-07T16:16:22Z

@jasonaden Updated the code to be conflict free to master, also fixed tslint errors, which was quite stupid, because somehow a local tslint does not show the errors.

mitchellwills · 2017-11-10T17:14:08Z

Any updates here? I'd love to see this PR land

jasonaden · 2017-12-21T23:47:21Z

I took another look at this PR and have some different thoughts on it.

Right now you're doing applyRedirects which is an async operation. But in the case of a URL to RouterStateSnapshot, I think we would want that to be sync. If you were to try to call the method with a URL where the RouterStateSnapshot doesn't exist, we would simply return null.

I can't think of a use case where we actually want this method to be async. If you can think of one, let me know. Otherwise we should convert this to be sync instead.

smurfy · 2017-12-22T10:12:35Z

@jasonaden well i think async makes sense, because it is possible, that not all routes are available yet. due to async modules.

Other than that, the main logic needed is in the recognize function anyways. I added the applyRedirects call, so all internal works are similiar to a router.navigate but instead of really navigating to the resolved RouterStateSnapshot it returns it.

ashwinirajput · 2018-01-03T06:25:57Z

@jasonaden how you are using applyRedirects

bioub · 2018-01-11T15:02:45Z

Please note that this wouldn't work for Lazy Loaded modules...

smurfy · 2018-01-11T17:58:37Z

Please note that this wouldn't work for Lazy Loaded modules...

well it does not trigger the loading of all modules to get all routes, but if the module is already loaded it works.

bevrard · 2018-03-01T17:14:36Z

Any news on this?

mleibman · 2018-05-30T21:25:28Z

Ping. This would be very useful indeed.

killianhale-work · 2018-10-02T19:11:41Z

@smurfy - This would be very useful indeed and is exactly what I'm looking for. Can you update this to the latest version of angular?
@jasonaden - Looks like the git history was rewritten but that your initial change requests were made. Care to update your response?

Thanks!

yelhouti · 2018-10-02T22:07:21Z

I really need this two...

yelhouti · 2018-10-02T22:53:55Z

Hey @jasonaden , I'm doing this as soon as I have your answer.
Do you want me to modify recognize and make it return a RouterStateSnapshot then export it through the router, which will involve modifying all the calls/tests...
Or just write an new function syncRecognize which has all the content of recognize, and make recognize call it and don't touch the other code (calls/tests)?
Thanks in advance

jasonaden · 2018-10-09T23:09:19Z

We're trying to clean up the oldest PRs now, and this is the oldest Router PR (sorry for all the delays).

Overall it looks good. There have been a number of changes to router.ts and this should be rebased. You should also use .pipe instead of the old .call syntax we were using when you wrote this.

Other than that, I believe this is good to go.

smurfy · 2018-10-10T05:33:37Z

Ok, try to rebase it today

jasonaden · 2018-10-11T22:18:10Z

@smurfy Let me know if you need help on the rebase here. I'm sometimes on Gitter, or you can DM me on Twitter at jasonaden1

JoostK · 2018-10-10T07:49:45Z

packages/router/test/router.spec.ts

+         ]);
+
+         r.urlToRouteStateSnapshot('/a').then(
+             (s) => { expect(s.root.firstChild !.data).toEqual({state: 'i am A'}); });


Hmm, do these asynchronous expectations actually work reliably? I think nowadays we should use async/await instead, as doing so will return a Promise to jasmine which it will wait for.

No they don't, tested it

yelhouti · 2018-11-13T04:06:44Z

Any news on this please?

yelhouti · 2018-12-19T11:50:32Z

@jasonaden since you declined the merge how can I help to fix this please so you accept it. I really need this feature. Thanks.

atscott

Please also address the comments in #15826 (comment)

atscott · 2020-08-10T15:56:54Z

packages/router/src/router.ts

+          recognize(this.rootComponentType, this.config, appliedUrl, this.serializeUrl(appliedUrl)),
+          (snapshot: any) => { return snapshot; });
+    });
+    return toPromise.call(snapshot$);


Why not just leave it as an Observable and let the caller decide if they want to use toPromise or not?

atscott · 2020-08-10T15:58:28Z

packages/router/test/router.spec.ts

+             path: 'a',
+             component: ComponentA,
+             data: {state: 'i am A'},
+             children: [{path: 'b', component: ComponentB, data: {state: 'i am B'}}]


Let's also add a test for lazy loaded modules. Even if it doesn't work (though I don't immediately see why it wouldn't), the test documents the behavior. It does seem to me like this would actually trigger the lazy loading. If that's true, this needs to be documented.

atscott · 2021-01-14T21:31:59Z

Closing due to lack of activity. If you're still interested in working on this change, please open a new PR (and address the feedback from this one) and I'll be happy to review it. Thanks!

angular-automatic-lock-bot · 2021-02-14T16:34:10Z

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.}

googlebot added the cla: no label Apr 7, 2017

googlebot added cla: yes and removed cla: no labels Apr 7, 2017

smurfy force-pushed the fix-15750 branch 2 times, most recently from 268b80d to 7d1eb7d Compare April 10, 2017 08:11

vicb added area: router feature Label used to distinguish feature request from other issues labels Apr 10, 2017

jasonaden suggested changes Jul 13, 2017

View reviewed changes

jasonaden self-assigned this Jul 13, 2017

jasonaden added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 13, 2017

smurfy force-pushed the fix-15750 branch from 7d1eb7d to e30b9ad Compare July 27, 2017 16:20

smurfy force-pushed the fix-15750 branch 3 times, most recently from c8d2d13 to 6ea35fa Compare August 7, 2017 16:06

feat(router): added method to convert an Url to RouterStateSnapshot

0150e35

smurfy force-pushed the fix-15750 branch from 6ea35fa to 0150e35 Compare August 7, 2017 16:10

jasonaden added the freq2: medium label Dec 21, 2017

JoostK reviewed Oct 11, 2018

View reviewed changes

rorymcgit approved these changes Oct 25, 2018

View reviewed changes

jasonaden added this to the needsTriage milestone Jan 29, 2019

yelhouti mentioned this pull request May 11, 2019

refactor(router): make recognize synchronous to expose it later #30410

Closed

14 tasks

atscott mentioned this pull request Aug 6, 2020

feat(router): add method to convert an Url to RouterStateSnapshot #30411

Closed

14 tasks

atscott unassigned jasonaden Aug 6, 2020

pullapprove bot requested a review from atscott August 6, 2020 20:57

atscott requested changes Aug 10, 2020

View reviewed changes

atscott closed this Jan 14, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 14, 2021

Conversation

smurfy commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Apr 7, 2017

Uh oh!

smurfy commented Apr 7, 2017

Uh oh!

googlebot commented Apr 7, 2017

Uh oh!

bijanajamlou commented May 5, 2017

Uh oh!

tartexs commented Jun 6, 2017

Uh oh!

jasonaden left a comment

Choose a reason for hiding this comment

Uh oh!

jasonaden Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

jasonaden commented Jul 26, 2017

Uh oh!

smurfy commented Jul 26, 2017

Uh oh!

smurfy commented Jul 27, 2017

Uh oh!

smurfy commented Aug 7, 2017

Uh oh!

mitchellwills commented Nov 10, 2017

Uh oh!

jasonaden commented Dec 21, 2017

Uh oh!

smurfy commented Dec 22, 2017

Uh oh!

ashwinirajput commented Jan 3, 2018

Uh oh!

bioub commented Jan 11, 2018

Uh oh!

smurfy commented Jan 11, 2018

Uh oh!

bevrard commented Mar 1, 2018

Uh oh!

mleibman commented May 30, 2018

Uh oh!

killianhale-work commented Oct 2, 2018

Uh oh!

yelhouti commented Oct 2, 2018

Uh oh!

yelhouti commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonaden commented Oct 9, 2018

Uh oh!

smurfy commented Oct 10, 2018

Uh oh!

jasonaden commented Oct 11, 2018

Uh oh!

JoostK Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

yelhouti May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yelhouti commented Nov 13, 2018

Uh oh!

yelhouti commented Dec 19, 2018

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

atscott Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

atscott Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

atscott commented Jan 14, 2021

Uh oh!

angular-automatic-lock-bot bot commented Feb 14, 2021

smurfy commented Apr 7, 2017 •

edited

Loading

yelhouti commented Oct 2, 2018 •

edited

Loading

yelhouti May 11, 2019 •

edited

Loading