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.

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

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

Closed
wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Sep 9, 2020

…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
atscott added a commit to atscott/angular that referenced this pull request Sep 9, 2020
This change contains the test from angular#38780. The fix is already present in master
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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.
@atscott
Copy link
Contributor Author

atscott commented Sep 10, 2020

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

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.

None yet

4 participants