Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Router: Patch version of #40029 #40315

Closed
wants to merge 6 commits into from
Closed

Router: Patch version of #40029 #40315

wants to merge 6 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Jan 5, 2021

See #40029

@atscott
refactor(router): Use for...of rather than mapChildrenIntoArray hel…
7a5d083 
…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
refactor(router): Update recognize to return null when there is n…
8a37ba2 
…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
refactor(router): make recognize synchronous (angular#40029)
60098a1 
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
fix(router): Ensure named outlets with empty path parents are recogni…
68f0858 
…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
refactor(router): Extract common functions for config matching (angul…
4465f76 
…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 atscott added the target: patch This PR is targeted for the next patch release label Jan 5, 2021
@google-cla google-cla bot added the cla: yes label Jan 5, 2021
@ngbot ngbot bot added this to the Backlog milestone Jan 5, 2021
@atscott
fix(router): apply redirects should match named outlets with empty pa…
a1b4b95 
…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 josephperrott added the action: merge The PR is ready for merge by the caretaker label Jan 5, 2021
@josephperrott
Copy link
Member

Closed by commit f542e4e

josephperrott pushed a commit that referenced this pull request Jan 5, 2021
@atscott @josephperrott
refactor(router): Use for...of rather than mapChildrenIntoArray hel…
ed42c93 
…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
@atscott @josephperrott
refactor(router): Update recognize to return null when there is n…
141fd3a 
…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
@atscott @josephperrott
refactor(router): make recognize synchronous (#40029) (#40315)
718aa36 
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
@atscott @josephperrott
fix(router): Ensure named outlets with empty path parents are recogni…
c722c43 
…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
@atscott @josephperrott
refactor(router): Extract common functions for config matching (#40029)…
68a9511 
… (#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
@atscott @josephperrott
fix(router): apply redirects should match named outlets with empty pa…
f542e4e 
…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

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

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router cla: yes PullApprove: disable target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@atscott @josephperrott