Sitelet https://github.com/angular/angular/pull/38780
Skip to content

fix(router): Ensure routes are processed in priority order and only i… - #38780

Closed
atscott wants to merge 1 commit into
angular:10.1.xfrom
atscott:fixRouter
Closed

atscott wants to merge 1 commit into
angular:10.1.xfrom
atscott:fixRouter

Conversation

@atscott

@atscott atscott commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

…f needed

There is a slight difference between map...concatAll and concatMap
in that the latter (concatMap) will ensure that the computations are
executed in-order and only if needed while the former may execute the
map body of all items if they do not emit immediately. That is, if the stream
is
from([a, b, c]).pipe(map(v => of(v).pipe(delay(1))), concatAll(), first())
the map body will execute for all of a, b, and c.
However, the following will only execute the concatMap body for a
from([a, b, c]).pipe(concatMap(v => of(v).pipe(delay(1))), first())

See https://stackblitz.com/edit/rxjs-cvwxyx

fixes #38691

Note: rather than using defer in the canLoad computation, I went with concatMap to avoid issues with missing the defer in other places down the line. Note that this is the second time a bug has occurred because the computations of the route may happen immediately (the place I removed defer from in this PR).

Also note that concatMap is already being used in the master branch but I will still need to create another PR to add the test from this one to master.

…f needed

There is a slight difference between `map`...`concatAll` and `concatMap`
in that the latter (`concatMap`) will ensure that the computations are
executed in-order and only if needed while the former may execute the
`map` body of all items if they do not emit immediately. That is, if the stream
is
`from([a, b, c]).pipe(map(v => of(v).pipe(delay(1))), concatAll(), first())`
the `map` body will execute for all of `a`, `b`, and `c`.
However, the following will only execute the `concatMap` body for `a`
`from([a, b, c]).pipe(concatMap(v => of(v).pipe(delay(1))), first())`

See https://stackblitz.com/edit/rxjs-cvwxyx

fixes angular#38691
@ngbot ngbot Bot added this to the needsTriage milestone Sep 9, 2020
@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Sep 9, 2020

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

atscott added a commit to atscott/angular that referenced this pull request Sep 9, 2020
This change contains the test from angular#38780 and also removes `defer` from
the `apply_redirects` logic because the change that introduced
`concatMap` instead of `map`...`concatAll` makes `defer` unnecessary.
Comment thread packages/router/src/apply_redirects.ts
@atscott

atscott commented Sep 10, 2020

Copy link
Copy Markdown
Contributor Author

presubmit

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 10, 2020
AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
…f needed (#38780)

There is a slight difference between `map`...`concatAll` and `concatMap`
in that the latter (`concatMap`) will ensure that the computations are
executed in-order and only if needed while the former may execute the
`map` body of all items if they do not emit immediately. That is, if the stream
is
`from([a, b, c]).pipe(map(v => of(v).pipe(delay(1))), concatAll(), first())`
the `map` body will execute for all of `a`, `b`, and `c`.
However, the following will only execute the `concatMap` body for `a`
`from([a, b, c]).pipe(concatMap(v => of(v).pipe(delay(1))), first())`

See https://stackblitz.com/edit/rxjs-cvwxyx

fixes #38691

PR Close #38780
@AndrewKushnir

Copy link
Copy Markdown
Contributor

This PR is now merged.

AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020
This change contains the test from #38780 and also removes `defer` from
the `apply_redirects` logic because the change that introduced
`concatMap` instead of `map`...`concatAll` makes `defer` unnecessary.

PR Close #38781
@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 Oct 11, 2020
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: router cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants