fix(router): Pass correct component to canDeactivate checks when using two sibling router-outlets by martinsik · Pull Request #36302 · angular/angular

martinsik · 2020-03-29T10:23:43Z

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?

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?

Issue Number: #34614

There's an edge case where if I use two (or more) sibling <router-outlet>s in two (or more) child routes where their parent route doesn't have a component then preactivation will trigger all canDeactivate checks with the same component because it will use wrong OutletContext.

{
 path: 'a',
 children: [
   {
     path: 'b1',
     component: BlankCmp,
     canDeactivate: ['RecordingDeactivate'],
   },
   {
     path: 'b2',
     canDeactivate: ['RecordingDeactivate'],
     component: SimpleCmp,
     outlet: 'aux',
   },
 ],
},
{
  path: 'c',
  component: BlankCmp,
},

Navigating from '/a/(b1//aux:b2)' to '/c' will call both canDeactivate guards with BlankCmp.

Replication steps are described in #34614.
Updated demo for Angular 9: https://stackblitz.com/edit/angular-named-outlet-enygni?file=app%2Fapp.component.html

What is the new behavior?

canDeactivate check is called with correct component.

Does this PR introduce a breaking change?

Yes
No

Other information

This fix will pass extra parameter to deactivateRouteAndItsChildren that is used in case a route is without a component because there's probably no other way to determine the correct OutletContext beforehand. The child routers are iterated inside deactivateRouteAndItsChildren where it can access only child OutletContexts. However, in this case the router-outlets are siblings.

martinsik · 2020-03-30T08:22:55Z

The failing test is because the bundle size increased but it looks like the expected size was already outdated. It expects 226144 bytes but when I run the same integration test locally the same bundle has 226631. With this PR the same bundle has 226659.

atscott · 2020-03-30T20:26:54Z

presubmit

atscott · 2020-03-30T22:22:40Z

global presubmit passed

atscott · 2020-03-30T22:41:56Z

As for the size failure, those need to be updated every once in a while. Lucky PR authors increase the size just enough to break the 500B threshold and have to update the _payload-limits.json :)

martinsik · 2020-04-02T11:57:40Z

I had to rebase to master and rerun yarn gulp format on integration.spec.ts to pass ci/circleci: lint. Although, I don't know why it made so many changes.

atscott · 2020-04-02T16:08:03Z

@martinsik - thanks for the update. The formatting changes are expected as clang format was just updated in c5c57f6.
Going to run a subset of presubmits as a final sanity check. This PR should get marked for merge later today.

atscott

Just two nits to remove null assertion operator now that the type includes null

atscott · 2020-04-03T17:54:02Z

FYI - I also just ran the size test locally at master and got 226611 for the size so the increase is only 48 between the PR and master.

kara

LGTM for size changes (given that it's only 48)

…g two or more sibling router-outlets (#36302) fixes #34614 There's an edge case where if I use two (or more) sibling <router-outlet>s in two (or more) child routes where their parent route doesn't have a component then preactivation will trigger all canDeactivate checks with the same component because it will use wrong OutletContext. PR Close #36302

…hen using two or more sibling router-outlets (angular#36302)" This reverts commit 80e6c07.

atscott · 2020-04-17T23:21:59Z

Hi @martinsik - A failure was detected internally so this PR is being rolled back in #36697. I added a test in #36699 to cover the failing use-case.

martinsik · 2020-04-18T10:49:14Z

@atscott Should I open another PR when that test is merged and I fix this PR?

…hen using two or more sibling router-outlets (#36302)" (#36697) This reverts commit 80e6c07. PR Close #36697

matsko · 2020-04-20T17:57:51Z

Hey @martinsik yes please if you could rebase + update this PR with the test that Andrew provided then we can reland this fix.

martinsik · 2020-04-22T19:01:41Z

#36699

This PR adds test case to cover a failure that was detected after merging angular#36302. That commit will be reverted and will need a new PR that does not cause this test to fail.

This PR adds test case to cover a failure that was detected after merging #36302. That commit will be reverted and will need a new PR that does not cause this test to fail. PR Close #36699

petebacondarwin · 2021-04-17T12:39:58Z

@martinsik - sorry that this PR got stuck. Could you rebase on master to check that it is still working after the new test was added, and to resolve the conflict? Then I think we should be able to land it.

petebacondarwin · 2021-05-07T15:17:12Z

Actually this PR has been resolved via other commits.

angular-automatic-lock-bot · 2021-06-07T16:27:43Z

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

googlebot added the cla: yes label Mar 29, 2020

pullapprove Bot requested a review from atscott March 29, 2020 10:23

martinsik force-pushed the issue-34614 branch from 692292c to 37ea397 Compare March 29, 2020 10:49

atscott added the area: router label Mar 30, 2020

ngbot Bot added this to the needsTriage milestone Mar 30, 2020

atscott reviewed Mar 30, 2020

View reviewed changes

Comment thread packages/router/src/utils/preactivation.ts Outdated

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

pullapprove Bot requested a review from kara April 2, 2020 08:53

martinsik force-pushed the issue-34614 branch 6 times, most recently from 225d5bf to f505eaa Compare April 2, 2020 10:14

atscott added the target: major This PR is targeted for the next major release label Apr 2, 2020

atscott reviewed Apr 2, 2020

View reviewed changes

Comment thread packages/router/src/utils/preactivation.ts Outdated

Comment thread packages/router/src/utils/preactivation.ts Outdated

martinsik force-pushed the issue-34614 branch from f505eaa to 04f43ce Compare April 3, 2020 08:45

atscott approved these changes Apr 3, 2020

View reviewed changes

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 3, 2020

kara added the action: presubmit The PR is in need of a google3 presubmit label Apr 3, 2020

kara approved these changes Apr 3, 2020

View reviewed changes

atscott removed the action: presubmit The PR is in need of a google3 presubmit label Apr 3, 2020

atscott added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed action: presubmit The PR is in need of a google3 presubmit target: major This PR is targeted for the next major release labels Apr 9, 2020

atscott closed this in 80e6c07 Apr 9, 2020

matsko added a commit to matsko/angular that referenced this pull request Apr 17, 2020

Revert "fix(router): pass correct component to canDeactivate checks w…

78560a3

…hen using two or more sibling router-outlets (angular#36302)" This reverts commit 80e6c07.

atscott mentioned this pull request Apr 17, 2020

test(router): add canDeactivate test for forChild route #36699

Closed

matsko added a commit that referenced this pull request Apr 20, 2020

Revert "fix(router): pass correct component to canDeactivate checks w…

b5e92b9

…hen using two or more sibling router-outlets (#36302)" (#36697) This reverts commit 80e6c07. PR Close #36697

matsko added a commit that referenced this pull request Apr 20, 2020

Revert "fix(router): pass correct component to canDeactivate checks w…

722d939

…hen using two or more sibling router-outlets (#36302)" (#36697) This reverts commit 80e6c07. PR Close #36697

matsko reopened this Apr 20, 2020

matsko removed the action: merge The PR is ready for merge by the caretaker label Apr 20, 2020

mike-co mentioned this pull request Apr 21, 2020

canDeactivate passes wrong component as first argument angular/router#447

Open

atscott 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 Jun 15, 2020

petebacondarwin closed this May 7, 2021

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jun 7, 2021

Uh oh!

Conversation

martinsik commented Mar 29, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

martinsik commented Mar 30, 2020

Uh oh!

Uh oh!

atscott commented Mar 30, 2020

Uh oh!

atscott commented Mar 30, 2020

Uh oh!

atscott commented Mar 30, 2020

Uh oh!

martinsik commented Apr 2, 2020

Uh oh!

atscott commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atscott commented Apr 3, 2020

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Apr 17, 2020

Uh oh!

martinsik commented Apr 18, 2020

Uh oh!

matsko commented Apr 20, 2020

Uh oh!

martinsik commented Apr 22, 2020

Uh oh!

petebacondarwin commented Apr 17, 2021

Uh oh!

petebacondarwin commented May 7, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Jun 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

atscott commented Apr 2, 2020 •

edited

Loading