feat(router): take only the first emitted value of every resolver #44573

dimakuba · 2021-12-28T13:49:02Z

The router now waits for the resolvers to complete even they emitted at
least one value. The changes make it possible to take only the first
emitted value of every resolver and proceed the navigation.

atscott

I certainly don't disagree that this likely should have been written this way in the first place. That said, this change is at the very least a breaking change and will need to wait until the next major version. You'll need to add a BREAKING CHANGES footer note to the commit message.

Depending on internal test results, we'll have to determine if this is worth the migration effort. I imagine there should be little to no failures so we should be able to do this, but you never really know until the test results come back.

Also, I think you will need to update the documentation. There are two places that mention that observables must complete in the tutorial. While those mention guards rather than resolvers, I believe that documentation is incorrect or outdated since I see observable.pipe(first()) in the guard execution code. I thought there was also somewhere that mentions this for resolvers, but I can't find it. Maybe I was thinking of this:

angular/aio/content/guide/router-tutorial-toh.md

Line 2444 in 8ebc946

1. Returning an empty `Observable` in at least one resolver cancels navigation.

atscott · 2022-01-04T17:13:53Z

packages/router/test/operators/resolve_data.spec.ts

@@ -23,6 +23,7 @@ describe('resolveData operator', () => {
        {provide: 'resolveTwo', useValue: (a: any, b: any) => of(2)},
        {provide: 'resolveFour', useValue: (a: any, b: any) => 4},
        {provide: 'resolveEmpty', useValue: (a: any, b: any) => EMPTY},
+        {provide: 'resolveZero', useValue: (a: any, b: any) => interval()},


nit: Given that this doesn't really resolve 0, but is rather an interval, I think this should be renamed.

atscott · 2022-01-05T00:59:18Z

@dimakuba Can you also explain the rationale behind this change? What does this help or solve? Is it just for consistency with guards?

Because the Observable isn't subscribed to and published, this change is functionally just taking the first value rather than the last one before completion. Generally I would expect this change to be a no-op for all existing use-cases. I can't think of an existing use-case that makes sense to emit multiple values with the intent of resolving just the last one. I might categorize this as a refactor rather than a feat unless you have an example of a feature use-case this change would unlock.

dimakuba · 2022-01-05T07:29:07Z

@dimakuba Can you also explain the rationale behind this change? What does this help or solve? Is it just for consistency with guards?

Because the Observable isn't subscribed to and published, this change is functionally just taking the first value rather than the last one before completion. Generally I would expect this change to be a no-op for all existing use-cases. I can't think of an existing use-case that makes sense to emit multiple values with the intent of resolving just the last one. I might categorize this as a refactor rather than a feat unless you have an example of a feature use-case this change would unlock.

Yes, you're right, this change is mostly for consistency with guards to take only the first emitted value and complete the Observable automatically rather then manually for "complete-less" observables.

atscott · 2022-01-07T00:02:20Z

@dimakuba That's fair and I think this is worth making consistent.

Action items for merging:

Please update the docs as mentioned in the previous comment. This should resolve #44643 (while you're at it, please update the sentence mentioned in that bug to say "resolvers" instead of guards).
Add "resolves #44643" to the footer of your commit message so that issue is closed when this PR is merged
Update the commit to be refactor rather than feat. Another option might be fix(router):... but this isn't exactly a bug...
Add the breaking changes note in the footer. i.e. "BREAKING CHANGE: previously, resolvers.... with this change, the router now only takes the first value...This is now consistent with Observables returned by guards..."
Update the commit message to explain why you made this change (i.e. consistency with guards)
AI for me: There were 4 failures in the global presubmit tests. I'll need to investigate and resolve those before this can be merged.

atscott · 2022-02-03T18:59:42Z

@dimakuba The main branch is now targeting v14 so we can land this change when the action items are addressed.

dimakuba · 2022-02-12T11:09:27Z

Please update the docs as mentioned in the previous comment. This should resolve #44643 (while you're at it, please update the sentence mentioned in that bug to say "resolvers" instead of guards).
Add "resolves #44643" to the footer of your commit message so that issue is closed when this PR is merged
Update the commit to be refactor rather than feat. Another option might be fix(router):... but this isn't exactly a bug...
Add the breaking changes note in the footer. i.e. "BREAKING CHANGE: previously, resolvers.... with this change, the router now only takes the first value...This is now consistent with Observables returned by guards..."
Update the commit message to explain why you made this change (i.e. consistency with guards)
AI for me: There were 4 failures in the global presubmit tests. I'll need to investigate and resolve those before this can be merged.

dimakuba · 2022-02-12T12:11:23Z

@atscott the only last point left to resolve

atscott · 2022-02-28T22:03:43Z

g3 cleanup is finished. green TGP

ngbot · 2022-02-28T22:04:39Z

I see that you just added the action: merge label, but the following checks are still failing:
     status "ci/circleci: test_docs_examples" is failing
     status "pullapprove" is failing
     status "google3" is pending
     2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

… to make it consistent with guards The router used to wait for the resolvers to complete and take the last value. The changes here take only the first emitted value of every resolver and proceed the navigation. This matches how other guards work in the `Router` code. Resolves angular#44643 BREAKING CHANGE: Previously, resolvers were waiting to be completed before proceeding with the navigation and the Router would take the last value emitted from the resolver. The router now takes only the first emitted value by the resolvers and then proceeds with navigation. This is now consistent with `Observables` returned by other guards: only the first value is used.

jessicajaniuk · 2022-03-01T17:12:39Z

This PR was merged into the repository by commit c967976.

angular-automatic-lock-bot · 2022-04-01T16:11:32Z

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

… to make it consistent with guards (angular#44573) The router used to wait for the resolvers to complete and take the last value. The changes here take only the first emitted value of every resolver and proceed the navigation. This matches how other guards work in the `Router` code. Resolves angular#44643 BREAKING CHANGE: Previously, resolvers were waiting to be completed before proceeding with the navigation and the Router would take the last value emitted from the resolver. The router now takes only the first emitted value by the resolvers and then proceeds with navigation. This is now consistent with `Observables` returned by other guards: only the first value is used. PR Close angular#44573

pullapprove bot requested a review from AndrewKushnir Dec 28, 2021

AndrewKushnir requested a review from atscott Jan 4, 2022

AndrewKushnir added comp: router feature labels Jan 4, 2022

ngbot bot added this to the Backlog milestone Jan 4, 2022

pullapprove bot removed the request for review from atscott Jan 4, 2022

atscott requested changes Jan 4, 2022

View changes

atscott added action: presubmit breaking changes labels Jan 4, 2022

atscott added target: major refactoring and removed feature labels Jan 6, 2022

atscott removed this from the Backlog milestone Jan 7, 2022

atscott added this to the v14-candidates milestone Jan 7, 2022

dimakuba force-pushed the take-first-value-of-resolver branch 2 times, most recently from 4780ba6 to a14de51 Compare Feb 12, 2022

angular-robot bot added the flag: breaking change label Feb 12, 2022

atscott force-pushed the take-first-value-of-resolver branch from a14de51 to 97f9635 Compare Feb 14, 2022

atscott mentioned this pull request Feb 22, 2022

Usage of rxjs7.x scheduled() on Resolvers causes Angular routing to hang #45141

Closed

atscott added action: merge and removed action: presubmit labels Feb 28, 2022

atscott approved these changes Feb 28, 2022

View changes

atscott force-pushed the take-first-value-of-resolver branch from 97f9635 to 389d926 Compare Feb 28, 2022

atscott added action: merge and removed action: merge labels Feb 28, 2022

atscott removed the request for review from AndrewKushnir Feb 28, 2022

jessicajaniuk closed this in angular/angular@c967976 Mar 1, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 1, 2022

feat(router): take only the first emitted value of every resolver #44573

feat(router): take only the first emitted value of every resolver #44573

dimakuba commented Dec 28, 2021 •

edited

atscott left a comment

atscott Jan 4, 2022

atscott commented Jan 5, 2022

dimakuba commented Jan 5, 2022

atscott commented Jan 7, 2022 •

edited

atscott commented Feb 3, 2022

dimakuba commented Feb 12, 2022 •

edited by atscott

dimakuba commented Feb 12, 2022

atscott commented Feb 28, 2022

ngbot bot commented Feb 28, 2022

jessicajaniuk commented Mar 1, 2022

angular-automatic-lock-bot bot commented Apr 1, 2022

feat(router): take only the first emitted value of every resolver #44573

feat(router): take only the first emitted value of every resolver #44573

Conversation

dimakuba commented Dec 28, 2021 • edited

atscott left a comment

atscott Jan 4, 2022

Choose a reason for hiding this comment

atscott commented Jan 5, 2022

dimakuba commented Jan 5, 2022

atscott commented Jan 7, 2022 • edited

atscott commented Feb 3, 2022

dimakuba commented Feb 12, 2022 • edited by atscott

dimakuba commented Feb 12, 2022

atscott commented Feb 28, 2022

ngbot bot commented Feb 28, 2022

jessicajaniuk commented Mar 1, 2022

angular-automatic-lock-bot bot commented Apr 1, 2022

dimakuba commented Dec 28, 2021 •

edited

atscott commented Jan 7, 2022 •

edited

dimakuba commented Feb 12, 2022 •

edited by atscott