Skip to content

feat(router): Cascading route support (4.3.x) #18163

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

Closed
wants to merge 1 commit into from

Conversation

matthewerwin
Copy link

See #16416

This is rebased for merging into 4.3.x

@jasonaden FYI includes your code review request for changes on the original 4.0.x pull request.

Added cascading routes capability to the router, added new test cases.
Previous behavior resulted in dead routes (e.g. direct navigation to a
route with canActivate=>false resulted in blank page). Additionally this
feature improvement provides conditional routing based on params and other
information that could be used to dynamically enable/disable a route while
ensuring the user always falls through to a valid route.

One existing test was modified which needed to verify depth-first-order on matching. recognize() can sequence all matches now. However, the records will still be returned by recognize() with the same depth-first-order.

Passes all latest tests and is backwards compatible.
@Toxicable
Copy link

Toxicable commented Jul 17, 2017

@matthewerwin Why are there 3 copies of this PR?
Changes cannot be made to prior version of Angular, only newer versions.
Also since this is a feature it would require a bump in the minor version number, meaning that if this is merged it'll be for v5.x since there shouldn't be a 4.4.x

@matthewerwin matthewerwin changed the title feat(router): Cascading route support when guards=>false feat(router): Cascading route support (4.3.x) Jul 17, 2017
@matthewerwin
Copy link
Author

@Toxicable there were differences due to a number of things that happened to Angular codebase since the 4.0.x version -- this PR has been around for a long time. Since I'm unsure where it makes most sense for the Angular team to merge (4.3.x would be closest to 5.x, or we can issue a PR against master).

The software change is backwards compatible so I don't know if that'll be a factor related to where it should be merged. I'm happy to delete whatever PR's don't make sense and issue against whatever branch it does...just trying to make sure that it can be managed as desired by Angular team and I'll make sure any future PR's are in alignment with the decision on how this is merged.

@Toxicable
Copy link

Toxicable commented Jul 17, 2017

I don't mean sound pedantic here, but like I said before, it cannot be merged into those older versions. Even though it's a old PR, what version it goes into is based on when it's merged not when the PR was made.

Even though it's backwards compatible that's only because of semantic versioning which you would be breaking by merging this into 4.0, 4.1, 4.2 or 4.3 therefore the only option is v5, unless there is a 4.4 which there shouldn't be.

@jasonaden jasonaden self-requested a review July 17, 2017 19:40
@jasonaden jasonaden self-assigned this Jul 17, 2017
@jasonaden jasonaden added area: router action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 17, 2017
@jasonaden
Copy link
Contributor

Let's just move forward with this version since it's the one that's synced to the more recent version of Angular.

@matthewerwin You can go ahead and do the compare against master branch. We only merge to that branch, and the 4.x.x branches are just for creating/tagging the releases.

@jasonaden
Copy link
Contributor

@matthewerwin Ignore above comment.

Let's go back to #16416 as the main PR since that has the most history.

Please rebase that one against master and submit the compare against master.

Closing this PR.

@jasonaden jasonaden closed this Jul 17, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: router cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants