Change Detection when Declaration and Insertion parent views don't match

Author: Miško Hevery

Prior Discussion

Definitions




























@Component({
  selector: 'lib-comp',
  template: `
    LibComp: {{greeting}}!
    <ng-container
        [ngTemplateOutlet]="template"
        [ngTemplateOutletContext]="{$implicit: greeting}">
    </ng-container>
  `
})
class LibComp {
  @Input()
  template: TemplateRef;
  greeting:string = "Hello";
}

@Component({
  template: `
    AppComp: {{name}}!
    <ng-template #myTmpl let-greeting>
      {{greeting}} {{name}}!
    </ng-template>
    <lib-comp [template]="myTmpl"></lib-comp>
  `
})
class AppComp {
  name: string = "world";
}

Declaration View: The LView where the <ng-template> was declared. In our case it would be the LView of AppComp.
Insertion View: The LView where the instance of <ng-template> has been inserted. In our example it would be the LView of LibComp.
Transplant LView: Transplant LView is the LView where the insertion point LView[PARENT] and declaration LView[DECLARATION_VIEW] do not match. In our example this would be LView created by the LibComp from the TemplateRef which was declared at AppComp.

Mental Model

The developer of application (AppComp) and the library author (LibComp) are two different developers, therefore the assumptions of one should not leak into the other. For example the LibComp should be free to decide if the LibComp is OnPush or Always without effecting the kinds of assumptions AppComp has. To put it differently LibComp should be able to change from Always to OnPush without breaking CD for the transplanted LView.
- Implication of this is that the CD strategy is attached to the transplanted template and not to the insertion LView. (Author of the template controls the CD strategy.)

`AppComp`	`LibComp`	Outcome
`Always`	`Always`	Run CD through all `LView`s on each tick.
`Always`	`OnPush`	Changing `AppComp.name` should trigger the update of the binding `{{greeting}} {{name}}!` regardless if `LibComp` is dirty.
`OnPush`	`Always`	Changing `LibComp.greeting` should trigger the update of the `{{greeting}} {{name}}!` regardless if the `AppComp` is dirty.
`OnPush`	`OnPush`	Changing `LibComp.greeting` or `AppComp.name` should trigger the CD of the template.

What the above table suggest is that the transplantedLView should be CDed if either the AppComp.name or the LibComp.greeting changes. In other words the transplant LView is subject to being marked dirty by either the AppComp or LibComp being marked dirty.

NOTE:

If the transplant LView is detached it should not be CDed. (This behavior is different from VE)

Suppose both AppComp and LibComp are OnPush. Which views should be CDed when either AppComp or LibComp change state and are marked dirty?

`AppComp` `.name`	`LibComp` `.greeting`	Transplanted `{{greeting}} {{name}}!`	`AppComp: {{name}}!`	`LibComp: {{greeting}}!`	Outcome
-	-	-	-	-	when neither component is marked dirty a CD run should not update any of the bindings.
mutate	-	Update	Update	-	When `AppComp` is marked dirty than `{{greeting}} {{name}}!`, `AppComp: {{name}}!` should be updated, but `LibComp: {{greeting}}!` should not be updated.
-	mutate	Update	-	Update	When `LibComp` is marked dirty than `{{greeting}} {{name}}!`, `LibComp: {{greeting}}!` should be updated, but `AppComp: {{name}}!` should not be updated.
mutate	mutate	Update	Update	Update	When `LibComp` and `AppComp` is marked dirty than `{{greeting}} {{name}}!`, `LibComp: {{greeting}}!`, and `AppComp: {{name}}!` should be updated.

The implications from the above table are that:

AppComp being marked dirty should CD its transplanted LView but it should not cause CD in LibComp.
- This is a breaking change with respect to VE, which (I believe) causes the CD in LibComp as well.
LibComp being marked dirty should CD its transplanted LView but it should not cause CD in AppComp.
- (I belive) this is consistent with VE behavior.
A detached transplanted LView should not be CD in either case.
- This is a breaking change with respect to VE, which (I believe) causes the CD even if the transplanted LView is detached.

Proposed Fix

The implication from the above discussion is that the <ng-template> needs to keep track of the transplanted LView instances so that it can invoke change detection on them from both declared as well as inserted view.

LView currently keeps track of all components in LView[CHILD_HEAD|CHILD_TAIL|NEXT] (in addition to LView). This is unnecessary because we already keep track of all child components in TView.components. Removing them would speed up creation and updates as tracking and iterating requires time.
All transplanted LViews should be added the the declared LView linked-list.
- Only add if LView[PARENT] !== LView[DECLARATION_VIEW (transplanted) and LView[PARENT] !== null (attached).
Component's linked-list should also contain LView[PREV] to make it efficient to remove transplanted LViews. This is an additional cost to the LView as it will increase its header size by one slot.
Change Detection Processing:
- During CD of LibComp we should always visit any (including transplanted) view at insertion point (current behavior).
- During CD of declaration LView we should also visit the transplanted views in the LView[CHILD_HEAD|CHILD_TAIL|NEXT].
  - The implication of this is that in the case of transplanted LView we will run the CD on the transplanted LView twice. Once from the AppComp and once from the LibComp. We could guard against this, but that would require additional work and checks, and given that this scenario is rare, I don't think it is worth worrying about this double checking.

Open Issues

This does not solve the problem of having the transplanted View inserted in the LibComp and than have the LibCom be detached. In such a situation one would expect that the template does not get CD under any use case, but in our case it would get CDed in the case when the AppCom would get marked as dirty.

Pawel Kozlowski

2019/11/05 10:33:42

AFAIK this tree (with component views) is used during the view tree destruction process to invoke destroy hooks in the "right" order. Changing this might have impact on the destroy functionality. Proposed data structure change should be evaluated from this point of view (Edited)

2019/11/05 10:35:56

Misko, I could think of an alternative approach where we focus on "markForCheck" rather than "detectChanges": https://docs.google.com/presentation/d/1AlaZN7znhprEUkhr760i8DKwDTVaerU7HGOgrszYfsc/edit?usp=sharing (Edited)

2019/11/05 11:03:11

Although this approach wouldn't work for "back-propagation"... I'm not sure what VE does in this case but it _seems_ like this is a valid use-case (?). Probably a bummer... (Edited)

Miško Hevery

2019/11/05 16:53:36

Hmm, I had a quick view, and I did not see that it was used. I saw that we have a separate array for destruction, but yes we should investigate. (Edited)

2019/11/05 16:54:40

Yes back propagation is the issue. It is solvable, but makes the code more complex. I think the question is how correct do we want to be. (Edited)

2019/11/05 16:54:52

I would like to agree that 1) current state is wrong 2) What the correct state should be 3) How correct should the fix be. (Edited)

Kara

2019/11/05 23:42:05

Can we test some of these assumptions about VE on stackblitz? (Edited)

2019/11/06 00:48:38

1) It looks like VE correctly skips CD for LibComp already 2) Confirmed this is consistent with VE 3) Detached transplanted LView does not get change detected in View Engine. The bug is that if a parent is detached from CD, the transplanted view is still change detected. (Edited)

2019/11/06 00:59:07

Don't we store containers in the CHILD_HEAD linked list? That wouldn't be covered by TView.components (Edited)

2019/11/06 01:02:15

e.g. https://github.com/angular/angular/blob/master/packages/core/src/render3/node_manipulation.ts#L179 (Edited)

2019/11/06 01:04:45

How rare is this case? Material uses the template transplanting pattern pretty extensively, IIRC (Edited)

2019/11/06 01:07:10

When you say "declared LView linked list", do you mean the CHILD_HEAD list or something else? A bit confused because above you suggest removing it (Edited)