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

feat(router): add UrlSegment[] to CanLoad interface #13127

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@marcuskrahl
Contributor

marcuskrahl commented Nov 29, 2016

Please check if the PR fulfills these requirements

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)

The Route object as passed to implementations of CanLoad which only provides minimal information on the page which should be navigated to. (see #12411)

What is the new behavior?

Additionally to Route an UrlSegment[] is passed to implementations of CanLoad as a second parameter. It contains the array of path elements the user tried to navigate to before canLoad is evaluated.

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

[ ] Yes
[x] No

Other information:

CanLoad now defines UrlSegment[] as a second parameter of the function.
Users can store the initial url segments and refer to them later, e.g. to go
back to the original url after authentication via router.navigate(urlSegments).
Existing code still works as before because the second function parameter
does not have to be defined.

Closes #12411

@jhuntoo

This comment has been minimized.

jhuntoo commented May 13, 2017

@vicb any chance of getting this one merged soon ? It solves a valid use-case (capturing the url without having to load a lazy module), and it appears to be a non breaking change.

@jasonaden

Looks good overall. Please make a couple minor updates, rebase, and let's get this merged. Sorry it's been sitting for a while.

@@ -273,11 +273,12 @@ class ApplyRedirects {
}
}
private getChildConfig(injector: Injector, route: Route): Observable<LoadedRouterConfig> {
private getChildConfig(injector: Injector, route: Route, urlSegments: UrlSegment[]):

This comment has been minimized.

@jasonaden

jasonaden Jun 6, 2017

Contributor

For consistency with other methods receiving UrlSegment[], please name the param segments.

@@ -383,15 +384,16 @@ class ApplyRedirects {
}
}
function runGuards(injector: Injector, route: Route): Observable<boolean> {
function runGuards(
injector: Injector, route: Route, urlSegments: UrlSegment[]): Observable<boolean> {

This comment has been minimized.

@jasonaden

jasonaden Jun 6, 2017

Contributor

Name it segments.

* route: Route
* ): Observable<boolean>|Promise<boolean>|boolean {
* return this.permissions.canLoadChildren(this.currentUser, route);
* canLoad(route: Route, urlSegments: UrlSegment[]): Observable<boolean>|Promise<boolean>|boolean

This comment has been minimized.

@jasonaden

jasonaden Jun 6, 2017

Contributor

This formatting looks bad. Can you reformat more similarly to how it was before?

@jasonaden

This comment has been minimized.

Contributor

jasonaden commented Jul 16, 2017

@marcuskrahl I went ahead and tried to get this rebased for you. But it's not working after the changes I made. Basically, when you submitted this PR all the files were under modules/@angular/router and now they are under packages/router. Here is the branch where I rebased. If you want this commit, take it. Otherwise, can you get this rebased and sent back in?

@googlebot

This comment has been minimized.

googlebot commented Jul 19, 2017

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jul 19, 2017

@googlebot

This comment has been minimized.

googlebot commented Jul 19, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 19, 2017

@googlebot

This comment has been minimized.

googlebot commented Jul 19, 2017

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot googlebot added cla: no and removed cla: yes labels Jul 19, 2017

@googlebot

This comment has been minimized.

googlebot commented Jul 19, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 19, 2017

@marcuskrahl

This comment has been minimized.

Contributor

marcuskrahl commented Jul 20, 2017

@jasonaden I made the necessary changes and everything should work as expected again. The unit tests run successfully. However the travis e2e test seems to be broken for everyone for now so I am waiting for this to be fixed.

@jasonaden

This comment has been minimized.

Contributor

jasonaden commented Jul 26, 2017

@marcuskrahl Okay, thanks.

Please rebase again. We had an issue that messed up the commit histories on many PRs. So this one needs a new rebase as it's showing hundreds of commits.

@ghost

This comment has been minimized.

ghost commented Oct 11, 2017

What's the status of this?

@MickL

This comment has been minimized.

MickL commented Oct 20, 2017

Please merge

@picosam

This comment has been minimized.

picosam commented Dec 11, 2017

Any idea about when this should be merged?

@marcuskrahl

This comment has been minimized.

Contributor

marcuskrahl commented Jan 14, 2018

I rebased again. Giving this another try to get merged

@digaus

This comment has been minimized.

digaus commented Feb 6, 2018

When will it be merged?

@ngbot

This comment has been minimized.

ngbot bot commented Mar 20, 2018

Hi @marcuskrahl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot

This comment has been minimized.

ngbot bot commented Mar 20, 2018

Hi @marcuskrahl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

feat(router): add UrlSegment[] to CanLoad interface
CanLoad now defines UrlSegment[] as a second parameter of the function.
Users can store the initial url segments and refer to them later, e.g. to go
back to the original url after authentication via router.navigate(urlSegments).
Existing code still works as before because the second function parameter
does not have to be defined.

Closes #12411
@marcuskrahl

This comment has been minimized.

Contributor

marcuskrahl commented Mar 29, 2018

I fixed the conflicts so this PR should be good to merge again

@PoiScript

This comment has been minimized.

PoiScript commented Apr 18, 2018

Any news? I'm hoping that this can be merged and included in the v6.

@ratidzidziguri

This comment has been minimized.

ratidzidziguri commented May 22, 2018

Any update on this? we also need this fix to be available.

@ngbot

This comment has been minimized.

ngbot bot commented Aug 16, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    failure status "code-review/pullapprove" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment