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

Downgraded component not reacting to user events in a lazy loaded module using downgradeModule approach #22581

Closed
FDIM opened this Issue Mar 4, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@FDIM
Copy link
Contributor

FDIM commented Mar 4, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Downgraded component that is part of a lazy loaded module is not reacting to user events when using downgradeModule approach.

Expected behavior

It should work normally as if it's part of the main module / not lazy loaded.

Minimal reproduction of the problem with instructions

Now:

  • open developer tools in chrome, go to sources tab and locate static.js
  • put a breakpoint on line 526 (var doDowngrade = function (injector) {) and reload the page
  • on first hit app-root component is initialized and you can observe that needsNgZone value is true
  • on second hit, it's a component that is loaded from lazy loaded module but now needsNgZone is set to false.
  • If you set it's value to true using console and continue - the component will work as expected. I mean clicking click to expand will work and reveal the text that is hidden with *ngIf

All the code related to the example can be seen in this commit: FDIM/angular-hybrid-lazy-load-example@918d750

What is the motivation / use case for changing the behavior?

To make it possible to use downgradeComponent inside lazy loaded module with a workaround described here: #17490

In short the workaround is about setting $$$angularInjectorController on some parent (in e.g. route resolve) which forces downgrade component to use that value as injector and thus correctly locate the component. As I understand this is working OK when UpgradeModule is used, but not with downgradeModule.

I propose the following fix:

  • setting REQUIRE_INJECTOR_NEEDS_NG_ZONE variable to ?^^$$angularInjectorNeedsNgZone
  • using it as follows:
    image

This way it would be possible to extend previously mentioned workaround and force the variable to be true.

Environment

Angular version: 5.2.7

Browser: any

P.S. I would have used stackblitz for the example, but the lazy loading there is not working: https://stackblitz.com/github/fdim/angular-hybrid-lazy-load-example

@FDIM

This comment has been minimized.

Copy link
Contributor Author

FDIM commented Mar 14, 2018

@gkalpak since I'll be making a PR for another issue, would you accept one for this as well? provided that we agree on the solution first. This is the reason for us having a local copy of the library

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 14, 2018

I actually think this is a more general problem with multiple ng1/ng2 layers (not specific to lazy-loading).
I.e.:

<ng1-comp-a>
  <ng2-downgraded-comp-a>
    <ng1-upgraded-comp-b>
      <ng2-downgraded-comp-b> <!-- This one will not get properly change-detected. -->

Let me look into it.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 14, 2018

Yes, that is indeed the case (demo).
I think we are basing our use of needsNgZone on wrong assumptions, which mostly hold, but break in the ng1 > ng2 > ng1 > ng2 case. We need to refactor things a bit.

@FDIM

This comment has been minimized.

Copy link
Contributor Author

FDIM commented Mar 14, 2018

Well, I tried the example and if I apply my fix (setting needsNgZone to true with debugger) it seem to work as well.
Could it be that the value must be true always when downgradeModule is used? or are you aware of something breaking if that would be the case?
At least to me it sort of makes sense that it should if all angularjs components are running outside angular zone.

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Mar 14, 2018

AFAICT, this is what makes the difference (and it does indeed depend on needsNgZone). You can also see that changing propagateDigest to false for the deeply nested component "fixed" CD:

directive(CounterComponent.selector, downgradeComponent({
  component: CounterComponent,
  propagateDigest: false,
})).

The AngularJS code may or may not run inside the Angular zone (e.g. an upgraded component usually runs inside the zone), so that cannot be a deciding factor.
I too believe that needsNgZone should be always true when downgradeModule() is used.

I was planning to work on a PR, but if you want to take it on, be my guest 😃

@FDIM

This comment has been minimized.

Copy link
Contributor Author

FDIM commented Mar 15, 2018

Well, I'd like to close the other PR first and then if this is still pending I'll have a look :)

@FDIM

This comment has been minimized.

Copy link
Contributor Author

FDIM commented Mar 19, 2018

I just tested it out, and if I pass propagateDigest: false it works in my example as well.

@gkalpak if you say otherwise, I'll try to make a PR that will add another flag to the if you pointed out here that will check if downgradeModule is used

EDIT: I couldn't resist and wanted to sleep well so I prepared a PR :)

FDIM added a commit to FDIM/angular that referenced this issue Mar 19, 2018

fix(upgrade): downgraded component not always attached to app
In a case like ng1 > ng2-downgraded > ng1-upgraded > ng2-downgraded, the last component would not be attached to the application and would not be part of change detection

Closes angular#22581
@Naooki

This comment has been minimized.

Copy link

Naooki commented Apr 10, 2018

We've been facing this issue on our project, and were struggling to find the reason of this CD problem.
That's great that you've managed to find it, great job.
Would be glad to see the fix as soon as possible.
Btw, setting propagateDigest to false for downgraded components doesn't actually solve the problem in our case... Only manual setting of needsNgZone to false does the thing.

@FDIM

This comment has been minimized.

Copy link
Contributor Author

FDIM commented Apr 12, 2018

@Naooki What if you only pass true as first argument in your case here ? Does it work then?

@Naooki

This comment has been minimized.

Copy link

Naooki commented Apr 24, 2018

@FDIM Yeah. sorry for a delayed answer.
So I tired out passing true as the first argument to the setupInputs() call, but only this didn't actually fixes the issue in my case. To make this work I've done what you said AND removed needsNgZone here, as if needsNgZone was true.
. . .
After a short research I've found out that the probable reason for this is that we wrap the entire app with a dummy downgraded component, to bootstrap ng2 module and get rid of the Trying to get the Angular injector before bootstrapping an Angular module. error.
It appears that I've got the injector here, which contains the reference to the component-wrapper mentioned above, and therefore prevents setting needsNgZone to true.

@Naooki

This comment has been minimized.

Copy link

Naooki commented Apr 25, 2018

To be clear, my case is:
ng2(to bootstrap module and allow the usage of ng2 services in ng1) -> ng1 ... ng1-> ng2.
Also, I've tested out a simple case with: ng1 -> ng2, and everything works just fine.
So the problem for me is more like here. Seems like whenever I've got component structure like ... ng2 -> ng1 -> ng2 I get this parentInjector defined, and therefore don't set needsNgZone to true.

@gkalpak gkalpak modified the milestones: needsTriage, Backlog May 11, 2018

@ngbot ngbot bot modified the milestones: Backlog, needsTriage May 11, 2018

gkalpak added a commit to gkalpak/angular that referenced this issue 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 issue 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 issue 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 gkalpak referenced this issue Nov 27, 2018

Closed

fix(upgrade): several ngUpgradeLite fixes #27217

4 of 6 tasks complete

gkalpak added a commit to gkalpak/angular that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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

ngfelixl added a commit to ngfelixl/angular that referenced this issue 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.