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(upgrade): ensure downgraded components are created in the Angular… #27083

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@kegluneq
Copy link
Contributor

kegluneq commented Nov 13, 2018

… zone

PR Checklist

Please check if your PR fulfills the following requirements:

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?

When using upgrade, in rare circumstances components are created outside of the Angular zone.

This can happen when the DOM looks like:

<wrapper> <!-- Angular Component w/ content projection -->
   <ng-js> <!-- AngularJS Component w/ conditional rendering -->
      <zonetest ng-if="true"> <!-- Angular Compnent -->
        In the zone: {{ inTheZone }}
      </zonetest>
   </ng-js>
</wrapper>

Issue Number: N/A

What is the new behavior?

Components in the situation described above are created in the Angular zone.

Does this PR introduce a breaking change?

  • Yes
  • No

@googlebot googlebot added the cla: yes label Nov 13, 2018

@vikerman vikerman requested a review from gkalpak Nov 13, 2018

@ngbot ngbot bot added this to the needsTriage milestone Nov 13, 2018

@vikerman

This comment has been minimized.

Copy link
Contributor

vikerman commented Nov 13, 2018

@kegluneq - An upgrade test is failing in circleci ?

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 14, 2018

@vikerman can you try a rerun for me?

The failing test passes when I run it locally, failed on a timeout, and includes a call to browser.sleep.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 14, 2018

@vikerman there was a couple flaky tests, I rerun them and everything is "green" now.

@gkalpak
Copy link
Member

gkalpak left a comment

This sounds similar to #22581 (which #22869 is trying to solve).
That said, I'm not sure the fix is right.

I tried reproducing the issue with code similar to your testcase, but couldn't: StackBlitz project
Can you reproduce the issue in on StackBlitz?

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

@gkalpak I spent some time trying to get it to repro on StackBlitz and had no luck yesterday, but you're right, I think this issue is very similar to #22581 (sorry for not finding it).

FWIW the unit test added in this PR fails before and passes after the fix. I can keep trying on StackBlitz though.

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

@gkalpak , I declare defeat.

I'm not sure why the bug won't repro on StackBlitz, but it does in the unit test in the PR and in internal Google code I can't post. In both cases, the proposed fix results in proper function. I wish I knew exactly what was going on, but I can't figure out the exact cases when the downgradeComponent factory function is called outside of the Angular zone.

I do think the change in #22869 is logically equivalent to the fix in this PR.

What happens next? I can't spend any more time on this at the moment, so please let me know if I need to work out an alternative.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 15, 2018

I'll try to figure out why the test fails without the fix. OOC, have you tried whether #22869 fixes your issue.
(I am a little reluctant to add a fix without understanding what exactly it is fixing.)

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

I have not explicitly patched that change, but modulo naming #22869 is the same as this PR (that is, attachToApp === needsNgZone)

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

Sorry, I was mistaken, they are not the same change. I'll try patching and see if it fixes the issue.

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

Yes, confirmed that #22869 does fix the original issue. At the moment I can't check whether it fixes the unit test in this PR, but I could do so when I return home this evening.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 15, 2018

Thx for the extra info, @kegluneq.
I actually realized that there are several issues here (all related to downgradeModule()):

  1. Nested, async instantiated components are not created/destroyed inside the zone.
  2. Nested components are not change-detected properly.
  3. Nested components belonging to different downgraded modules will not work correctly.

I am working on a PR.

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Nov 15, 2018

Thanks @gkalpak , let me know if there's anything I can do to help.

@FDIM

This comment has been minimized.

Copy link
Contributor

FDIM commented Nov 16, 2018

It's quite nice to see that somebody else managed to reproduce the same issue! @gkalpak ping me if you'd like to test your solution in the project or if you want extra pair of eyes

gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 21, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they should not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 22, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they should not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Nov 23, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 23, 2018

@kegluneq, @FDIM: I have submitted #27217, that should fix the issue you were having (and a couple more).
I would be great if you could verify it by running against your apps. (In case it helps, I have uploaded the updated @angular/upgrade package here.)

@FDIM

This comment has been minimized.

Copy link
Contributor

FDIM commented Nov 27, 2018

@gkalpak I did a quick test and it seems to work! Any chance for #26024 to be incorporated as well?

@gkalpak gkalpak referenced this pull request Nov 27, 2018

Closed

fix(upgrade): several ngUpgradeLite fixes #27217

4 of 6 tasks complete
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Nov 27, 2018

Thx for verifying, @FDIM.
(For #26024, see #26024 (review) 😉)

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 5, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 10, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 12, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 13, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 13, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 14, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 19, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 20, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()`

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

matsko added a commit that referenced this pull request Dec 20, 2018

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()` (#27217)

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes #22581
Closes #22869
Closes #27083

PR Close #27217

@matsko matsko closed this in 326b464 Dec 20, 2018

@kegluneq

This comment has been minimized.

Copy link
Contributor Author

kegluneq commented Jan 7, 2019

@gkalpak this works for my use case as well. Thank you!

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(upgrade): correctly handle nested downgraded components with `dow…
…ngradeModule()` (angular#27217)

Previously, nested downgraded components would not be created/destroyed
inside the Angular zone (as they should) and they would not be wired up
correctly for change detection.

This commit ensures that ngUpgrade correctly detects whether this is an
ngUpgradeLite app (i.e. one using `downgradeModule()` instead of
`UpgradeModule`) and appropriately handles components, even if they are
nested inside other downgraded components.

Fixes angular#22581
Closes angular#22869
Closes angular#27083

PR Close angular#27217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.