Skip to content

fix(router): ensure named outlets with empty path parents are matched#40029

Closed
atscott wants to merge 9 commits intoangular:masterfrom
atscott:emptyoutletparent
Closed

fix(router): ensure named outlets with empty path parents are matched#40029
atscott wants to merge 9 commits intoangular:masterfrom
atscott:emptyoutletparent

Conversation

@atscott
Copy link
Copy Markdown
Contributor

@atscott atscott commented Dec 8, 2020

see individual commits

@atscott atscott requested a review from mhevery December 8, 2020 20:40
@google-cla google-cla bot added the cla: yes label Dec 8, 2020
@mhevery mhevery closed this in 61b9adb Dec 9, 2020
mhevery pushed a commit that referenced this pull request Dec 9, 2020
@atscott atscott reopened this Dec 9, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 9, 2020
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all of the new comments. It is a good start! Please keep comenting.

zarend pushed a commit to zarend/angular that referenced this pull request Dec 10, 2020
@atscott atscott force-pushed the emptyoutletparent branch 3 times, most recently from b05d434 to 44cefbc Compare December 11, 2020 01:30
@atscott atscott marked this pull request as ready for review December 11, 2020 01:30
@atscott atscott force-pushed the emptyoutletparent branch 3 times, most recently from e767ddb to 44486ef Compare December 11, 2020 01:34
@atscott atscott changed the title Emptyoutletparent fix(router): ensure named outlets with empty path parents are matched Dec 11, 2020
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Dec 11, 2020

presubmit

When stepping through the `recognize` algorithm, it is much easier to
follow when using a simple `for...of` rather than the helper
`mapChildrenIntoArray` with the passed closure. The only special thing that
`mapChildrenIntoArray` does is ensure the primary route appears first.
This change will have no affect on the result because `processChildren` later calls
`sortActivatedRouteSnapshots`, which does the same thing.
…o match

This commit updates the `recognize` algorithm to return `null` when a
segment does not match a given config rather than throwing an error.
This makes the code much easier to follow because the "no match" result
has to be explicitly handled rather than catching the error in very
specific places.
@atscott atscott force-pushed the emptyoutletparent branch 2 times, most recently from 9c71295 to b4e1b64 Compare December 11, 2020 01:52
@pullapprove pullapprove bot added the area: core Issues related to the framework runtime label Dec 11, 2020
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Dec 11, 2020

global presubmit

@atscott atscott added the target: patch This PR is targeted for the next patch release label Dec 18, 2020
@josephperrott josephperrott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 5, 2021
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…o match (#40029)

This commit updates the `recognize` algorithm to return `null` when a
segment does not match a given config rather than throwing an error.
This makes the code much easier to follow because the "no match" result
has to be explicitly handled rather than catching the error in very
specific places.

PR Close #40029
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
To make the tests suite easier to follow, `Recognize#apply` can be made
into a synchronous function rather than one that return an `Observable`.

Also, as a chore, remove as many `any` types as possible.

PR Close #40029
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…zed (#40029)

This commit updates the `recognize` algorithm to work with named outlets
which have empty path parents. For example, given the following config

```
  const routes = [
    {
      path: '',
      children: [
        {path: 'a', outlet: 'aux', component: AuxComponent}
    ]}
  ];
```

The url `/(aux:a)` should match this config. In order to do so, we need
to allow the children of `UrlSegmentGroup`s to match a `Route` config
for a different outlet (in this example, the `primary`) when it's an
empty path. This should also *only* happen if we were unable to find a
match for the outlet in the level above. That is, the matching strategy
is to find the first `Route` in the list which _matches the given
outlet_. If we are unable to do that, then we allow empty paths from
other outlets to match and try to find some child there whose outlet
matches our segment.

PR Close #40029
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
The `applyRedirects` and `recognize` algorithms have the same overall goal:
match a `UrlTree` with the application's `Routes` config. There are a
few key functions in these algorithms which can be shared rather than
duplicated between the two. This also makes it easier to see how the two
are similar and where they diverge.

PR Close #40029
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…th parents (#40029)

There are two parts to this commit:
1. Revert the changes from #38379. This change had an incomplete view of
how things worked and also diverged the implementations of
`applyRedirects` and `recognize` even more.
2. Apply the fixes from the `recognize` algorithm to ensure that named
outlets with empty path parents can be matched. This change also passes
all the tests that were added in #38379 with the added benefit of being
a more complete fix that stays in-line with the `recognize` algorithm.
This was made possible by using the same approach for `split` by
always creating segments for empty path matches (previously, this was
only done in `applyRedirects` if there was a `redirectTo` value). At the
end of the expansions, we need to squash all empty segments so that
serializing the final `UrlTree` returns the same result as before.

Fixes #39952
Fixes #10726
Closes #30410

PR Close #40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
…per (angular#40029)

When stepping through the `recognize` algorithm, it is much easier to
follow when using a simple `for...of` rather than the helper
`mapChildrenIntoArray` with the passed closure. The only special thing that
`mapChildrenIntoArray` does is ensure the primary route appears first.
This change will have no affect on the result because `processChildren` later calls
`sortActivatedRouteSnapshots`, which does the same thing.

PR Close angular#40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
…o match (angular#40029)

This commit updates the `recognize` algorithm to return `null` when a
segment does not match a given config rather than throwing an error.
This makes the code much easier to follow because the "no match" result
has to be explicitly handled rather than catching the error in very
specific places.

PR Close angular#40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
To make the tests suite easier to follow, `Recognize#apply` can be made
into a synchronous function rather than one that return an `Observable`.

Also, as a chore, remove as many `any` types as possible.

PR Close angular#40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
…zed (angular#40029)

This commit updates the `recognize` algorithm to work with named outlets
which have empty path parents. For example, given the following config

```
  const routes = [
    {
      path: '',
      children: [
        {path: 'a', outlet: 'aux', component: AuxComponent}
    ]}
  ];
```

The url `/(aux:a)` should match this config. In order to do so, we need
to allow the children of `UrlSegmentGroup`s to match a `Route` config
for a different outlet (in this example, the `primary`) when it's an
empty path. This should also *only* happen if we were unable to find a
match for the outlet in the level above. That is, the matching strategy
is to find the first `Route` in the list which _matches the given
outlet_. If we are unable to do that, then we allow empty paths from
other outlets to match and try to find some child there whose outlet
matches our segment.

PR Close angular#40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
…ar#40029)

The `applyRedirects` and `recognize` algorithms have the same overall goal:
match a `UrlTree` with the application's `Routes` config. There are a
few key functions in these algorithms which can be shared rather than
duplicated between the two. This also makes it easier to see how the two
are similar and where they diverge.

PR Close angular#40029
atscott added a commit to atscott/angular that referenced this pull request Jan 5, 2021
…th parents (angular#40029)

There are two parts to this commit:
1. Revert the changes from angular#38379. This change had an incomplete view of
how things worked and also diverged the implementations of
`applyRedirects` and `recognize` even more.
2. Apply the fixes from the `recognize` algorithm to ensure that named
outlets with empty path parents can be matched. This change also passes
all the tests that were added in angular#38379 with the added benefit of being
a more complete fix that stays in-line with the `recognize` algorithm.
This was made possible by using the same approach for `split` by
always creating segments for empty path matches (previously, this was
only done in `applyRedirects` if there was a `redirectTo` value). At the
end of the expansions, we need to squash all empty segments so that
serializing the final `UrlTree` returns the same result as before.

Fixes angular#39952
Fixes angular#10726
Closes angular#30410

PR Close angular#40029
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…per (#40029) (#40315)

When stepping through the `recognize` algorithm, it is much easier to
follow when using a simple `for...of` rather than the helper
`mapChildrenIntoArray` with the passed closure. The only special thing that
`mapChildrenIntoArray` does is ensure the primary route appears first.
This change will have no affect on the result because `processChildren` later calls
`sortActivatedRouteSnapshots`, which does the same thing.

PR Close #40029

PR Close #40315
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…o match (#40029) (#40315)

This commit updates the `recognize` algorithm to return `null` when a
segment does not match a given config rather than throwing an error.
This makes the code much easier to follow because the "no match" result
has to be explicitly handled rather than catching the error in very
specific places.

PR Close #40029

PR Close #40315
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
To make the tests suite easier to follow, `Recognize#apply` can be made
into a synchronous function rather than one that return an `Observable`.

Also, as a chore, remove as many `any` types as possible.

PR Close #40029

PR Close #40315
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…zed (#40029) (#40315)

This commit updates the `recognize` algorithm to work with named outlets
which have empty path parents. For example, given the following config

```
  const routes = [
    {
      path: '',
      children: [
        {path: 'a', outlet: 'aux', component: AuxComponent}
    ]}
  ];
```

The url `/(aux:a)` should match this config. In order to do so, we need
to allow the children of `UrlSegmentGroup`s to match a `Route` config
for a different outlet (in this example, the `primary`) when it's an
empty path. This should also *only* happen if we were unable to find a
match for the outlet in the level above. That is, the matching strategy
is to find the first `Route` in the list which _matches the given
outlet_. If we are unable to do that, then we allow empty paths from
other outlets to match and try to find some child there whose outlet
matches our segment.

PR Close #40029

PR Close #40315
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
… (#40315)

The `applyRedirects` and `recognize` algorithms have the same overall goal:
match a `UrlTree` with the application's `Routes` config. There are a
few key functions in these algorithms which can be shared rather than
duplicated between the two. This also makes it easier to see how the two
are similar and where they diverge.

PR Close #40029

PR Close #40315
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…th parents (#40029) (#40315)

There are two parts to this commit:
1. Revert the changes from #38379. This change had an incomplete view of
how things worked and also diverged the implementations of
`applyRedirects` and `recognize` even more.
2. Apply the fixes from the `recognize` algorithm to ensure that named
outlets with empty path parents can be matched. This change also passes
all the tests that were added in #38379 with the added benefit of being
a more complete fix that stays in-line with the `recognize` algorithm.
This was made possible by using the same approach for `split` by
always creating segments for empty path matches (previously, this was
only done in `applyRedirects` if there was a `redirectTo` value). At the
end of the expansions, we need to squash all empty segments so that
serializing the final `UrlTree` returns the same result as before.

Fixes #39952
Fixes #10726
Closes #30410

PR Close #40029

PR Close #40315
@angular-automatic-lock-bot
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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project area: core Issues related to the framework runtime area: router cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: high target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routing issue after upgrading from Angular 10 to Angular 11 bug(router) aux-routes not working with empty-path top-level routes

5 participants