fix(ivy): don't detect changes on detached child embedded views #34846
Conversation
crisbeto
changed the title
crisbeto
force-pushed
the
34816/detached-embedded-view-cd
branch
from
c511e61
to
b3b55de
Compare
crisbeto
added
comp: ivy
action: review
|
|
||
| TestBed.configureTestingModule({declarations: [App, ViewManipulation]}); | ||
| const fixture = TestBed.createComponent(App); | ||
| const vm: ViewManipulation = fixture.debugElement.childNodes[2].references['vm']; |
There was a problem hiding this comment.
super-nit / personal preference: in cases like this I usually do debugElement.query(By.directive(ViewManipulation)).injector.get(ViewManipulation) as I find this approach less "brittle". (BTW, for quite a bit of time I wanted to have a better API to do just this in TestBed but this is a separate story....).
There was a problem hiding this comment.
EDIT: I tried it out and query didn't manage to find the directive for some reason. Maybe because I'm trying to access it before the first change detection run?
Sure, I don't feel strongly about it. I did it this way, because the other unit tests were doing the same.
pkozlowski-opensource
added
action: cleanup
crisbeto
force-pushed
the
34816/detached-embedded-view-cd
branch
from
b3b55de
to
7613282
Compare
There was a problem hiding this comment.
I've addressed/replied to all of the feedback @pkozlowski-opensource.
|
|
||
| TestBed.configureTestingModule({declarations: [App, ViewManipulation]}); | ||
| const fixture = TestBed.createComponent(App); | ||
| const vm: ViewManipulation = fixture.debugElement.childNodes[2].references['vm']; |
There was a problem hiding this comment.
EDIT: I tried it out and query didn't manage to find the directive for some reason. Maybe because I'm trying to access it before the first change detection run?
Sure, I don't feel strongly about it. I did it this way, because the other unit tests were doing the same.
crisbeto
added
action: review
crisbeto
force-pushed
the
34816/detached-embedded-view-cd
branch
from
7613282
to
6b9881f
Compare
|
I've added the extra unit test. |
Fixes Ivy detecting changes inside child embedded views, even though they're detached. Note that there's on subtlety here: I made the changes inside `refreshDynamicEmbeddedViews` rather than `refreshView`, because we support detecting changes on a detached view (evidenced by a couple of unit tests), but only if it's triggered directly from the view's `ChangeDetectorRef`, however we shouldn't be detecting changes in the detached child view when something happens in the parent. Fixes angular#34816.
crisbeto
force-pushed
the
34816/detached-embedded-view-cd
branch
from
6b9881f
to
feac972
Compare
crisbeto
added
action: merge
AndrewKushnir
removed
the
action: presubmit
Fixes Ivy detecting changes inside child embedded views, even though they're detached. Note that there's on subtlety here: I made the changes inside `refreshDynamicEmbeddedViews` rather than `refreshView`, because we support detecting changes on a detached view (evidenced by a couple of unit tests), but only if it's triggered directly from the view's `ChangeDetectorRef`, however we shouldn't be detecting changes in the detached child view when something happens in the parent. Fixes #34816. PR Close #34846
…lar#34846) Fixes Ivy detecting changes inside child embedded views, even though they're detached. Note that there's on subtlety here: I made the changes inside `refreshDynamicEmbeddedViews` rather than `refreshView`, because we support detecting changes on a detached view (evidenced by a couple of unit tests), but only if it's triggered directly from the view's `ChangeDetectorRef`, however we shouldn't be detecting changes in the detached child view when something happens in the parent. Fixes angular#34816. PR Close angular#34846
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes Ivy detecting changes inside child embedded views, even though they're detached.
Note that there's one subtlety here: I made the changes inside
refreshDynamicEmbeddedViewsrather thanrefreshView, because we support detecting changes on a detached view (evidenced by a couple of unit tests), but only if it's triggered directly from the view'sChangeDetectorRef, however we shouldn't be detecting changes in the detached child view when something happens in the parent.Note: I had to bump the size limit, because it looks like master was right on the edge before these changes. I haven't introduced any new imports and the new logic is only a couple of lines so I doubt that these changes would've added too much to the size.
Fixes #34816.