fix(router): empty resolvers cancel navigation #24621

martinsik · 2018-06-21T23:23:55Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When a route has at least two resolvers (Observables) and both of them complete without any next item the last() operator throws an exception.

Issue Number: #24195

What is the new behavior?

When there's at least one empty resolver then navigation is canceled.

I think there are two ways this could work. When at least one resolver is empty or when all resolvers are empty. The original issue #24195 mentions the first option and it probably makes more sense because if I'm fine with one of the resolvers producing an empty response I can handle it in the resolver with eg. defaultIfEmpty operator.

Does this PR introduce a breaking change?

[X] Yes
[] No

~~This shouldn't be a breaking change because it's very unlikely that anybody was relying on the previous behavior.~~

Other information

I actually made this more simple because I used only forkJoin and map operators with single pipe() call.

I didn't want to update docs until I have approval from somebody competent that these changes make sense.

martinsik · 2020-01-06T10:53:58Z

@jasonaden Hey! Please, is there any chance to get this merged when I fix these merge conflicts?

BovineEnthusiast · 2020-02-10T21:02:28Z

Hey, any update on this?

atscott

One small nit but otherwise LGTM. Could you rebase this PR to the current master so we can run tests?

packages/router/test/integration.spec.ts

martinsik · 2020-03-25T14:27:24Z

@atscott I finally rebased this PR. I had to make some refactoring because Angular Router has changed a lot in the meantime.

I was was battling with bundle size of one integration test because I originally solved this by adding forkJoin() from RxJS but since that isn't used anywhere else in this package it increased its size. I rewrote it so I'm not using forkJoin() at all and I also replaced reduce() with takeLast() (reduce() is internally implemented as a combination of scan() and takeLast()) so at the end the bundle should be even smaller after my changes. However, I wasn't able to run these tests locally so I can't tell how much.

So finally this should be good to go.

atscott

Great, thanks for adding this fix! I ran a subset of the internal presubmits yesterday and they appeared to be passing. I'll need to run them all tonight just to be safe, but I think this should be pretty much good to go.

packages/router/src/operators/resolve_data.ts

atscott · 2020-03-25T17:10:16Z

packages/router/src/operators/resolve_data.ts

      return from(canActivateChecks)
          .pipe(
              concatMap(
                  check => runResolve(
                      check.route, targetSnapshot !, paramsInheritanceStrategy, moduleInjector)),
-              reduce((_: any, __: any) => _), map(_ => t));
+              takeLast(1), map(_ => t), );


Great catch here. Really weird way to have been using reduce before.

Yeah, it took me a while to figure out why is it even used there :).

atscott · 2020-03-25T17:11:52Z

packages/router/test/integration.spec.ts

@@ -1629,6 +1630,64 @@ describe('Integration', () => {
         expect(e).toEqual(null);
       })));

+    it('should not navigate when all resolvers return empty result',


Great tests. Thanks for adding these.

atscott · 2020-03-26T16:31:35Z

global presubmits appear to show some failures as a result of this CL. I'll have to investigate what's going on there.

martinsik · 2020-03-29T09:04:54Z

I added at least a few unit tests for resolveData operator using marble diagrams. While doing that I realised there's one more use-case where I have multiple canActivate checks (I always have at least one created somewhere in preactivation). So the way it works not is that it will cancel navigation if at least one resolver in any canActivate checks doesn't emit.

I'm suspicious that right now resolveData is running all resolvers for each canActivate check but I'll need to test this first.

martinsik · 2020-03-30T09:34:36Z

I was finally able to run integration tests. With cli-hello-world-lazy-rollup test that was previously failing because of increased bundle size the difference with this PR is 8 bytes. It went from 226631 to 226639.

atscott · 2020-03-30T16:23:11Z

I added at least a few unit tests for resolveData operator using marble diagrams. While doing that I realised there's one more use-case where I have multiple canActivate checks (I always have at least one created somewhere in preactivation). So the way it works not is that it will cancel navigation if at least one resolver in any canActivate checks doesn't emit.

Oh, good. You identified the internal test failure before I got a chance to tell you about it :)

I'm suspicious that right now resolveData is running all resolvers for each canActivate check but I'll need to test this first.

Hmmm, that does seem suspicious. That wasn't how it was working before, was it?

I was finally able to run integration tests. With cli-hello-world-lazy-rollup test that was previously failing because of increased bundle size the difference with this PR is 8 bytes. It went from 226631 to 226639.

Yea, the bundle size tests frequently need to be updated.

atscott · 2020-03-31T18:00:11Z

Hi @martinsik, I want to clarify what the current behavior is for resolvers returning EMPTY:

If all resolvers return EMPTY, a NavigationError occurs and navigation does not complete
If only one of two or more resolvers return EMPTY, the navigation does complete without error

This behavior can be confirmed by patching your two tests into master and running them. I think we can agree that the inconsistency is not desirable. Making this consistent, either by having both cases cancel navigation as is done in this PR or by allowing navigation to complete in both cases, is probably considered a breaking change because there could be applications that rely on the current behavior (knowingly or otherwise).

I've come across several cases internally where there were CanActivate checks that passed and then resolvers which have catchError and return EMPTY. In these cases, the current behavior is that the route is still activated and the data is undefined. With this PR, the navigation would instead be cancelled. It's not easy to know what the desired behavior is here. Maybe these navigations should be cancelled, but maybe the developers prefer the navigation to continue and handle the undefined, meaning the resolver has to be updated to return of(undefined) instead of EMPTY. Because of this, I'm concerned that we can't make this change without unknowingly breaking applications.

An argument could also be made for fixing this in a way which allows navigation to complete in all cases rather than cancelling it, though I'm on your side of the discussion there -- if a developer really wanted undefined data to be resolved, he/she could specifically return an observable of undefined.

Lastly, this behavior is worth documenting. Can you find an appropriate place in the router documentation on resolvers to add a note?

martinsik · 2020-04-17T11:30:21Z

@atscott Rebased and resolved.

atscott · 2020-04-20T19:55:57Z

global presubmit

atscott · 2020-04-24T23:43:39Z

Hi @martinsik, I've been working through more test failures that came out of this change and realized that this needs to be marked as a breaking change. Please add a note about this at the end of your commit message.

Also, please expand upon the description in the commit body so readers can quickly understand what's being changed and why.

Finally, please add a note (and maybe even a test) that this will affect any resolvers used in RouterModule.forChild - I noticed that the current behavior (without this PR) for a forChild route with one resolver still continues to the route because the inherited resolve values are {}. So this is similar to the case where there are multiple resolvers.

angular/packages/router/src/operators/resolve_data.ts

Line 49 in 2365bb8

...inheritedParamsDataResolve(futureARS, paramsInheritanceStrategy).resolve

The commit message should be something like

fix(router): cancel navigation when at least one resolver completes with no "next" emissions

This change aligns behavior for resolvers which return EMPTY. Currently...With this change, xyz

BREAKING CHANGE: Any resolver which return EMPTY will cancel navigation. If you want to allow the navigation to continue, you will need to update the resolvers to emit some value, (i.e. defaultIfEmpty(...), of(...), etc). xyz xyz

atscott · 2020-04-28T18:18:25Z

@martinsik - friendly ping to update the commit message, as mentioned in the last comment. Since this is a breaking change, we'll need to get it into a major release. If we miss v10, it will need to wait for v11.

…ith no "next" emission This change aligns behavior for resolvers which return EMPTY. Currently EMPTY resolvers have inconsistent behavior: - One resolver that returns EMPTY => won't navigate and just ends on ResolveStart router event. - Two resolvers where both return EMPTY => throws "Error: Uncaught (in promise): EmptyError: no elements in sequence" - Two resolvers where one returns a value and the other one returns EMPTY => Navigates successfully. With this change any EMPTY resolver will cancel navigation. BREAKING CHANGE: Any resolver which return EMPTY will cancel navigation. If you want to allow the navigation to continue, you will need to update the resolvers to emit some value, (i.e. defaultIfEmpty(...), of(...), etc). PR Close angular#24195

martinsik · 2020-05-03T17:31:38Z

@atscott I updated the commit message and added two more tests using RouterModule.forChild()

atscott · 2020-05-04T15:37:53Z

latest global presubmit - both failures are already failing at head.

atscott · 2020-05-04T15:40:50Z

caretaker note: Please merge and sync this one separately from others if possible.
This PR fixes an edge case path that could break routing in some instances. I've sent an internal announcement about this change and updated some resolvers by hand to mitigate the risk.

angular-automatic-lock-bot · 2020-06-04T16:10:27Z

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

…ith no "next" emission (angular#24621) This change aligns behavior for resolvers which return EMPTY. Currently EMPTY resolvers have inconsistent behavior: - One resolver that returns EMPTY => won't navigate and just ends on ResolveStart router event. - Two resolvers where both return EMPTY => throws "Error: Uncaught (in promise): EmptyError: no elements in sequence" - Two resolvers where one returns a value and the other one returns EMPTY => Navigates successfully. With this change any EMPTY resolver will cancel navigation. BREAKING CHANGE: Any resolver which return EMPTY will cancel navigation. If you want to allow the navigation to continue, you will need to update the resolvers to emit some value, (i.e. defaultIfEmpty(...), of(...), etc). PR Close angular#24195 PR Close angular#24621

googlebot added the cla: yes label Jun 21, 2018

mhevery added the area: router label Jul 12, 2018

jasonaden added this to the needsTriage milestone Jan 29, 2019

jasonaden self-requested a review January 29, 2019 22:08

rastafan mentioned this pull request Dec 8, 2019

Router/PreActivation: Multiple Resolvers resolving to empty observables cause uncaught EmptyError #24195

Closed

pullapprove bot requested review from atscott and removed request for jasonaden February 11, 2020 00:19

atscott reviewed Feb 11, 2020

View reviewed changes

packages/router/test/integration.spec.ts Outdated Show resolved Hide resolved

martinsik force-pushed the issue-24195 branch 3 times, most recently from 8dea202 to 4347a6a Compare March 24, 2020 11:26

atscott added the PR action: rerun CI at HEAD label Mar 24, 2020

ngbot bot removed the PR action: rerun CI at HEAD label Mar 24, 2020

martinsik force-pushed the issue-24195 branch 2 times, most recently from e6a9ec5 to 9b1dd2e Compare March 25, 2020 08:27

atscott reviewed Mar 25, 2020

View reviewed changes

martinsik force-pushed the issue-24195 branch from 70f44c3 to f9f503b Compare March 28, 2020 21:03

martinsik force-pushed the issue-24195 branch from f9f503b to e239f71 Compare March 29, 2020 10:32

atscott added risk: medium action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews type: bug/fix labels Mar 31, 2020

atscott removed this from the needsTriage milestone Apr 2, 2020

martinsik force-pushed the issue-24195 branch 3 times, most recently from c9fa018 to 1de4ab2 Compare April 17, 2020 09:55

atscott removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 17, 2020

atscott added risk: high target: major This PR is targeted for the next major release breaking changes and removed risk: medium labels Apr 24, 2020

martinsik force-pushed the issue-24195 branch 2 times, most recently from a1f20e0 to 36c7de3 Compare May 3, 2020 13:51

martinsik force-pushed the issue-24195 branch from 36c7de3 to 8da485e Compare May 3, 2020 17:04

atscott removed the request for review from kara May 4, 2020 15:38

atscott approved these changes May 4, 2020

View reviewed changes

atscott added risk: medium and removed action: presubmit The PR is in need of a google3 presubmit risk: high labels May 4, 2020

atscott added the action: merge The PR is ready for merge by the caretaker label May 4, 2020

alxhub closed this in d9c4840 May 4, 2020

IgorMinar mentioned this pull request May 16, 2020

fix(dev-infra): misc fixes for the compare master and patch script #37150

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): empty resolvers cancel navigation #24621

fix(router): empty resolvers cancel navigation #24621

martinsik commented Jun 21, 2018 •

edited

martinsik commented Jan 6, 2020 •

edited

BovineEnthusiast commented Feb 10, 2020

atscott left a comment

martinsik commented Mar 25, 2020

atscott left a comment

atscott Mar 25, 2020

martinsik Mar 27, 2020

atscott Mar 25, 2020

atscott commented Mar 26, 2020

martinsik commented Mar 29, 2020 •

edited

martinsik commented Mar 30, 2020

atscott commented Mar 30, 2020

atscott commented Mar 31, 2020

martinsik commented Apr 17, 2020

atscott commented Apr 20, 2020

atscott commented Apr 24, 2020 •

edited

atscott commented Apr 28, 2020

martinsik commented May 3, 2020 •

edited

atscott commented May 4, 2020

atscott commented May 4, 2020 •

edited

angular-automatic-lock-bot bot commented Jun 4, 2020

fix(router): empty resolvers cancel navigation #24621

fix(router): empty resolvers cancel navigation #24621

Conversation

martinsik commented Jun 21, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

martinsik commented Jan 6, 2020 • edited

BovineEnthusiast commented Feb 10, 2020

atscott left a comment

Choose a reason for hiding this comment

martinsik commented Mar 25, 2020

atscott left a comment

Choose a reason for hiding this comment

atscott Mar 25, 2020

Choose a reason for hiding this comment

martinsik Mar 27, 2020

Choose a reason for hiding this comment

atscott Mar 25, 2020

Choose a reason for hiding this comment

atscott commented Mar 26, 2020

martinsik commented Mar 29, 2020 • edited

martinsik commented Mar 30, 2020

atscott commented Mar 30, 2020

atscott commented Mar 31, 2020

martinsik commented Apr 17, 2020

atscott commented Apr 20, 2020

atscott commented Apr 24, 2020 • edited

atscott commented Apr 28, 2020

martinsik commented May 3, 2020 • edited

atscott commented May 4, 2020

atscott commented May 4, 2020 • edited

angular-automatic-lock-bot bot commented Jun 4, 2020

martinsik commented Jun 21, 2018 •

edited

martinsik commented Jan 6, 2020 •

edited

martinsik commented Mar 29, 2020 •

edited

atscott commented Apr 24, 2020 •

edited

martinsik commented May 3, 2020 •

edited

atscott commented May 4, 2020 •

edited