GeezerVison

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.

I signed it! (CLA)

CLAs look good, thanks!

FYI ci/circleci is failing the LINT on the commit message but that is for someone else's commit that's already on angular/4.0.x (INVALID COMMIT MSG: "release: cut the 4.0.1 release").

@vicb do you have any commentary on this since you've been involved quite a bit in the router?

on a separate note while I was in there I noticed that exceptions were used during the normal paths of execution vs architecting to return a complex object or using an observable/promise reject/error. I'm up for tackling that re-factoring if you guys are on board once this request is merged.

Super excited for this as I am converting a legacy application and initially thought this was how Guards worked so I am currently in a bind.

@the-destro if you get a chance can you pull this into your project and verify it works as you expect for your particular use-case? If not, I'd like to catch any specific conditions you're addressing while this pull request is pending merge.

Here is what you run to get this pre-merged package running:

npm uninstall @angular/router --save
npm install @angular/router --registry https://myget.org/F/snaptech/npm --save

That will run this build to test then anytime you run "npm install" after it will just revert to the main branch or you can re-run uninstall and install without specifying the separate registry.

@matthewerwin upon quick inspection it looks like it works exactly as I need! To give further context to my route setup: http://stackoverflow.com/questions/43919064/finely-specify-routes-in-angular-2

As I understood from the description, this can be used insted of #14515 right?

@alexbyk thanks for referencing that. Yes, this pull request solves #14515

Is there an ETA on when this can be merged/released?

@vicb what's the word? Anything I can do to get this merged in more quickly?

@vicb @mhevery Any update on feedback or merging? Maybe for Angular 5?

@jasonaden I squashed the commits and rebased against 4.0.x for this pull request. I also fixed the errors (The 4.0.x branch on angular errors on ./build.sh due to some things that appeared to be refactored/fixed in 4.1.x). I figured everyone would prefer 4.0.x builds without issues when pulling that branch.

Separately I've also created pull requests for 4.2.x and 4.3.x and all of them use the purely rxjs style flow instead of the recursive function call that was in the initial review. That should make comparisons between the existing guard code and the cascading flow guard code very easy to identify. I cleaned up everything you mentioned in the Code Review & with the change to maintain the recognize( ) signature that also reduced the total changes in the pull request. :-)

@jasonaden I'll make it happen today. Will issue the new PR against master and test out if the suggested change works.

@CaerusKaru thanks for the update -- this PR has been lingering forever and not sure why. We made all the changes originally requested by the angular team and the interior of the router hasn't changed much other than a minor refactor in all the time since the PR was initially entered.

Let me know how we can support getting this moved forward so I don't have to maintain separate binaries for our platforms.

@kara @IgorMinar happy new year! What steps do we need to take to get this on track for review?

@atscott I noticed to pull approve not request 22 days ago...any ETA?

This would be super useful if you are creating a new version of a feature and you want to keep it on the same route but have it protected by a feature flag guard.

@josh18 exactly. Now if they would just finish the review and merge it already 🤦‍♂️

@atscott thank you for the thorough review. I will have a chance to go through in detail over the next few days and respond to each comment or resolve as suggested.

As to your last separate remark on backwards compatibility that’s easily done with a flag and is actually a reason why recognize.ts was modified...if it returns a single result (which then fails) then behavior should be identical to the original code but I will double check it.

Related to the choice of two dimensional array that is a matter of per iteration and cumulative selection of routes. The design is to pipe all matching routes via the observable to allow fall-through evaluation in navigate. So likely that will need to remain the same but I will look at it very thoroughly & beef up some test cases as needed to trial other techniques if necessary.

Look forward to making this as robust as necessary so if you have extra test cases or other things to assist I’m happy to wrap into a secondary branch to verify before folding them in here

Thanks a million! Will turn around on my side as quickly as I can.

@atscott the PR review modifications are in & I also did a force push b/c CircleCI required it. I am however seeing a failure in the AIO router examples project on an E2E test scenario here.

I'll be able to look at it tomorrow but in the meantime if you can review and verify that the feedback you gave on the PR was interpreted accurately that would be greatly appreciated! If you happen to see what's going on with that E2E test lmk...I'm just too tired to see it clearly at the moment.

Again, thank you so much for taking the feedback and iterating on the approach here. I think we're much closer now to an implementation that we can easily explain and defend. On those lines, thinking about what it would take to get this submitted:

• Can you update some documentation and the guards section of the router guide? We'll want some input from someone on the docs team, maybe @gkalpak. I know there is at least one place isn't accurate when cascade is true: https://angular.io/guide/router#guards

If it returns false, the navigation process stops and the user stays put.

• The default behavior in this PR is for cascade to be true. There needs to be a config which controls this and the default should be cascade === false so we don't break existing router setups.

@atscott thanks for the fast turnaround time on the review. I added the code to make cascading choice via config and default to cascade = false to maintain original behavior and avoid potentially causing breaking changes.

I also added some documentation per your request & updated the router API to include the enable/disable of the cascade feature via the ExtraOptions available on the router config. Please advise if I missed anything.

global presubmit passed, meaning the default config (i.e. the current router behavior) is fully backwards compatible.

I believe the lint failure is unrelated. A rebase should resolve it. Would you mind investigating the other two failures and squashing the commits into one?

@atscott I went ahead and rebased and squashed to a single commit. The lint failure was due to an earlier rebase when lint failures were present in master and appears resolved in the latest rebase.

The other failures you refer to are also happening on master branch and appear to be Ivy integration tests. I pulled master branch and re-built locally and ran all tests and see the same failures on that branch as here. I would resolve them if I knew how but Ivy integration is beyond the scope of my experience on this project. My guess is that they're added by 3rd parties as test cases to Ivy to obtain backwards compatibility or related to unresolved edge cases.

@matthewerwin Looks like you just need to update the bundle size limits in this file.

@IgorMinar thanks for taking the time to post. @JoostK and @atscott both provided some great feedback these past weeks which has all been incorporated.

I understand what you're staying about non-trival new behavior. Take a look at the latest comparisons though and you will see that this is restricted to the presence of a cascade property on the router config. The changes to router.ts were also pared back substantially to reduce the surface area as well.

A last note about this is that actually the number of line changes is a bit deceiving. The guard block was all moved into a concatMap, the recognize.ts logic was moved into a loop, and a couple others like this result in the diff showing a lot of changes but much of that is essentially whitespace changes

@atscott and @IgorMinar - what are we going to do about this PR? It is now very stale and old. Perhaps it is not something we will ever integrate? Or perhaps it can be updated and merged? Either way it can't get into 12.0.x, so it would now be targeting 12.1.0 or later.

I’m happy to get it rebased in for latest. I feel like every time the internal team gets ready to merge it though there is some distraction. We are also very busy on our side to keep going through the process to prepare to merge only to have it fall off priority for v10, v11 or whatever

Just need someone to champion it on the Angular team to help us make sure it goes all the way through to save us all a lot of time. Let me know who I can work with and we will get this rolled up this week.

We recently landed the CanMatch feature which enables cascading routes through an additional guard on a Route. CanMatch doesn’t solve the “blank screen” on initial navigation issue, but that can be addressed with a another solution as well (partially addressed by #46026 and will be followed up with a feature to configure this behavior for other cases). The combination of the two should satisfy the use-cases that this PR was aiming to address.

While the implementation we went with in the end differed from the approach taken here, the attention and feedback from this PR was a major factor in driving discussions and ultimately the solution to these use-cases.

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