GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat(router): Cascading route support (4.3.x) by matthewerwin · Pull Request #18163 · angular/angular

Conversation

@matthewerwin

@matthewerwin matthewerwin commented Jul 17, 2017

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@matthewerwin
feat(router): Cascading route support when guards=>false
16d4cbb 
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.
@googlebot googlebot added the cla: yes label Jul 17, 2017
@Toxicable

Toxicable commented Jul 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

matthewerwin commented Jul 17, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Toxicable

Toxicable commented Jul 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

jasonaden commented Jul 17, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasonaden

jasonaden commented Jul 17, 2017

Copy link
Copy Markdown
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.
matthewerwin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasonaden jasonaden closed this Jul 17, 2017
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 12, 2019

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.

Reviewers

@jasonaden jasonaden Awaiting requested review from jasonaden

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@jasonaden jasonaden

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: router cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@matthewerwin @Toxicable @jasonaden @googlebot
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 single 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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.