Skip to content

fix(router): Ensure all outlets are used when commands has a prefix #39456

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
Closed
wants to merge 3 commits into from
Closed

fix(router): Ensure all outlets are used when commands has a prefix #39456

wants to merge 3 commits into from
+119 −17

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Oct 27, 2020 •
edited
Loading

Note: Easiest to review is per-commit.

Commit 1: A refactor only. No functional changes. Also documents behavior with tests and coherent example config.
Commit 2: Small change to allow tests from commit 1 to pass.

When there is a primary outlet present in the outlets map and the object is also prefixed
with some other commands, the current logic only uses the primary outlet and ignores
the others. This change ensures that all outlets are respected at the
segment level when prefixed with other commands.

Fixes issue that prevented navigation in #39401.
Partially addressed #13523 - That issue, along with #15718, stem from a general difficulty to change outlets because one has to specify the change using the correct segment 'level' (see tests named should not clear secondary outlet when at root and prefix is used and should not clear non-root secondary outlet when command is targeting root). Additionally, it's still not possible to change two outlets that appear at different levels in the config in a single navigation.

@atscott atscott added area: router target: patch This PR is targeted for the next patch release labels Oct 27, 2020
@atscott atscott requested a review from mhevery October 27, 2020 16:57
@ngbot ngbot bot modified the milestone: needsTriage Oct 27, 2020
@google-cla google-cla bot added the cla: yes label Oct 27, 2020
@atscott atscott added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 27, 2020
@atscott
Copy link
Contributor Author

atscott commented Oct 27, 2020

presubmit

@alfaproject
Copy link
Contributor

My opinion ofc but I think it makes more sense to add the xited tests from 1st commit in the second commit. Both of these commits are in this MR anyway but looking at commit by commit when this gets merged, the fix will be together with the tests for it

mhevery
mhevery approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

LGTM, with few nits

@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 29, 2020
@atscott
Copy link
Contributor Author

atscott commented Oct 29, 2020

@alfaproject - That's a fair suggestion and I think different people will have different opinions on the matter. I actually like having a set of tests that are commented out/ignored but can be enabled when a fix becomes available. My feeling is that tests act as a form of documentation and the xit tests document expected behavior but that the system does not work correctly (yet). The tests are still enabled when the fix is added. This also provides an easy way to review the change -- I can enable the test and see it fail, then add the commit and see it pass.

Either way, I'm not aware of any style guide on this, so I'll just leave it as-is.

@atscott atscott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 29, 2020
@atscott
refactor(router): Small refactor of createUrlTree and extra tests
acfce4a 
This commit has a small refactor of some methods in create_url_tree.ts
and adds some test cases, including two that will fail at the moment but
should pass. A follow-up commit will make use of the refactorings to fix
the test with minimal changes.
@atscott
fix(router): Ensure all outlets are used when commands have a prefix
c54f9e1 
When there is a primary outlet present in the outlets map and the object is also prefixed
with some other commands, the current logic only uses the primary outlet and ignores
the others. This change ensures that all outlets are respected at the
segment level when prefixed with other commands.
@atscott
fixup! refactor(router): Small refactor of createUrlTree and extra tests
5346092
@atscott atscott added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Oct 29, 2020
@atscott
Copy link
Contributor Author

atscott commented Oct 30, 2020

global 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 Oct 30, 2020
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
@atscott @josephperrott
refactor(router): Small refactor of createUrlTree and extra tests (#3…
3abf95d 
…9456)

This commit has a small refactor of some methods in create_url_tree.ts
and adds some test cases, including two that will fail at the moment but
should pass. A follow-up commit will make use of the refactorings to fix
the test with minimal changes.

PR Close #39456
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
@atscott @josephperrott
fix(router): Ensure all outlets are used when commands have a prefix (#…
b2f3952 
…39456)

When there is a primary outlet present in the outlets map and the object is also prefixed
with some other commands, the current logic only uses the primary outlet and ignores
the others. This change ensures that all outlets are respected at the
segment level when prefixed with other commands.

PR Close #39456
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
@atscott @josephperrott
refactor(router): Small refactor of createUrlTree and extra tests (#3…
8f36c21 
…9456)

This commit has a small refactor of some methods in create_url_tree.ts
and adds some test cases, including two that will fail at the moment but
should pass. A follow-up commit will make use of the refactorings to fix
the test with minimal changes.

PR Close #39456
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
@atscott @josephperrott
fix(router): Ensure all outlets are used when commands have a prefix (#…
85d5242 
…39456)

When there is a primary outlet present in the outlets map and the object is also prefixed
with some other commands, the current logic only uses the primary outlet and ignores
the others. This change ensures that all outlets are respected at the
segment level when prefixed with other commands.

PR Close #39456
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
@atscott @josephperrott
fix(router): Ensure all outlets are used when commands have a prefix (#…
3af585c 
…39456)

When there is a primary outlet present in the outlets map and the object is also prefixed
with some other commands, the current logic only uses the primary outlet and ignores
the others. This change ensures that all outlets are respected at the
segment level when prefixed with other commands.

PR Close #39456
@atscott atscott mentioned this pull request Nov 3, 2020
Closed
@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 Nov 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery

Assignees
No one assigned
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
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@atscott @alfaproject @mhevery