@deferred under a <template> is not removed when the <template> is

[leonsenft]

Consider the following example:

<button (click)="show = !show">Toggle</button>
<div *ngIf="show">
  <hello-world @deferred></hello-world>
</div>

Pressing the Toggle button will alternate between adding and removing the <hello-world> component from the DOM. This behavior is broken, if this example is changed so that the parent element of the @deferred component is itself a <template>:

<button (click)="show = !show">Toggle</button>
<template [ngIf]="show">
  <hello-world @deferred></hello-world>
</template>

Now rather than alternating between adding and removing the <hello-world> component, pressing the Toggle button will alternative between adding and doing nothing. This means new <hello-world> components will be repeatedly appended to the DOM.

[matanlurey]

Wow! That's interesting considering the de-sugaring process. Does this work:

<button (click)="show = !show">Toggle</button>
<template [ngIf]="show">
  <div>
    <hello-world @deferred></hello-world>
  </div>
</template>

... i.e. an exact representation of the first example using <template>?

Is the issue that @deferred doesn't work if its immediate parent is <template>?

[leonsenft]

I haven't tested, but I'd assume that example works since it's effectively equivalent to my first example.

Yes, I believe the issue is related to the immediately nested <template> elements.

<button (click)="show = !show">Toggle</button>
<template [ngIf]="show">
  <template> <!-- defer instantiated by generated code -->
    <hello-world></hello-world>
  </template>
</template>

[matanlurey]

Thanks! I'll investigate.

[matanlurey]

Looking at the following two snippets:

    <deferred-child-1 @deferred></deferred-child-1>
    <template [ngIf]="showDeferredChild">
      <deferred-child-2 @deferred></deferred-child-2>
    </template>
    <div *ngIf="showDeferredChild">
      <deferred-child-3 @deferred #queryMe></deferred-child-3>
    </div>

The code that is generated is:

class _ViewTestContainerComponent1 extends AppView<import1.TestContainerComponent> {
  import7.Element _el_0;
  AppView _compView_0;
  dynamic _DeferredChild1Component_0_5;
  _ViewTestContainerComponent1(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewTestContainerComponent0._renderType;
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    _compView_0 = deflib1.viewFactory_DeferredChild1Component0(this, 0);
    _el_0 = _compView_0.rootEl;
    _DeferredChild1Component_0_5 = deflib0.DeferredChild1Component();
    _compView_0.create(_DeferredChild1Component_0_5, []);
    init0(_el_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _compView_0.detectChanges();
  }

  @override
  void destroyInternal() {
    _compView_0?.destroy();
  }
}

class _ViewTestContainerComponent3 extends AppView<import1.TestContainerComponent> {
  import7.Element _el_0;
  AppView _compView_0;
  dynamic _DeferredChild2Component_0_5;
  _ViewTestContainerComponent3(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewTestContainerComponent0._renderType;
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    _compView_0 = deflib1.viewFactory_DeferredChild2Component0(this, 0);
    _el_0 = _compView_0.rootEl;
    _DeferredChild2Component_0_5 = deflib0.DeferredChild2Component();
    _compView_0.create(_DeferredChild2Component_0_5, []);
    init0(_el_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _compView_0.detectChanges();
  }

  @override
  void destroyInternal() {
    _compView_0?.destroy();
  }
}

and

class _ViewTestContainerComponent5 extends AppView<import1.TestContainerComponent> {
  import7.Element _el_0;
  AppView _compView_0;
  dynamic _DeferredChild3Component_0_5;
  _ViewTestContainerComponent5(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewTestContainerComponent0._renderType;
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    _compView_0 = deflib1.viewFactory_DeferredChild3Component0(this, 0);
    _el_0 = _compView_0.rootEl;
    _DeferredChild3Component_0_5 = deflib0.DeferredChild3Component();
    _compView_0.create(_DeferredChild3Component_0_5, []);
    init0(_el_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _compView_0.detectChanges();
  }

  @override
  void dirtyParentQueriesInternal() {
    import9.unsafeCast<ViewTestContainerComponent0>(parentView.parentView)._query_queryMe_1_0_isDirty = true;
  }

  @override
  void destroyInternal() {
    _compView_0?.destroy();
  }
}

... one class each.

[matanlurey]

The generated views themselves don't seem wrong. Let's look at the insertion points:

class ViewTestContainerComponent0 extends AppView<import1.TestContainerComponent> {
  ViewContainer _appEl_1;
  ViewContainer _appEl_2;
  NgIf _NgIf_2_9;
  ViewContainer _appEl_3;
  bool _query_queryMe_1_0_isDirty = true;
  NgIf _NgIf_3_9;
  ViewContainer _appEl_4;
  ViewContainer _appEl_5;
  static RenderComponentType _renderType;
  ViewTestContainerComponent0(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.component, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    rootEl = import7.document.createElement('test-container');
    _renderType ??= import8.appViewUtils.createRenderType((import9.isDevMode ? 'asset:third_party.dart_src.angular._goldens/test/_files/deferred/container_component.dart' : null), ViewEncapsulation.None, styles$TestContainerComponent);
    setupComponentType(_renderType);
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    final _rootEl = rootEl;
    final import7.HtmlElement parentRenderNode = initViewRoot(_rootEl);
    import7.Text _text_0 = import7.Text('\'');
    parentRenderNode.append(_text_0);
    final _anchor_1 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_1);
    _appEl_1 = ViewContainer(1, null, this, _anchor_1);
    TemplateRef _TemplateRef_1_7 = TemplateRef(_appEl_1, viewFactory_TestContainerComponent1);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_1, _TemplateRef_1_7);
    final _anchor_2 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_2);
    _appEl_2 = ViewContainer(2, null, this, _anchor_2);
    TemplateRef _TemplateRef_2_8 = TemplateRef(_appEl_2, viewFactory_TestContainerComponent2);
    _NgIf_2_9 = NgIf(_appEl_2, _TemplateRef_2_8);
    final _anchor_3 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_3);
    _appEl_3 = ViewContainer(3, null, this, _anchor_3);
    TemplateRef _TemplateRef_3_8 = TemplateRef(_appEl_3, viewFactory_TestContainerComponent4);
    _NgIf_3_9 = NgIf(_appEl_3, _TemplateRef_3_8);
    final _anchor_4 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_4);
    _appEl_4 = ViewContainer(4, null, this, _anchor_4);
    TemplateRef _TemplateRef_4_7 = TemplateRef(_appEl_4, viewFactory_TestContainerComponent6);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_4, _TemplateRef_4_7);
    final _anchor_5 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_5);
    _appEl_5 = ViewContainer(5, null, this, _anchor_5);
    TemplateRef _TemplateRef_5_7 = TemplateRef(_appEl_5, viewFactory_TestContainerComponent7);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_5, _TemplateRef_5_7);
    init(const [], null);
    return null;
  }

  @override
  void detectChangesInternal() {
    final import1.TestContainerComponent _ctx = ctx;
    _NgIf_2_9.ngIf = _ctx.showDeferredChild;
    _NgIf_3_9.ngIf = _ctx.showDeferredChild;
    _appEl_2.detectChangesInNestedViews();
    _appEl_3.detectChangesInNestedViews();
    if (!import8.AppViewUtils.throwOnChanges) {
      if (_query_queryMe_1_0_isDirty) {
        ctx.queryDeferredChild = import8.firstOrNull(_appEl_3.mapNestedViews((_ViewTestContainerComponent4 nestedView) {
          return nestedView._appEl_1.mapNestedViews((_ViewTestContainerComponent5 nestedView) {
            return [nestedView._DeferredChild3Component_0_5];
          });
        }));
        _query_queryMe_1_0_isDirty = false;
      }
    }
    _appEl_1.detectChangesInNestedViews();
    _appEl_4.detectChangesInNestedViews();
    _appEl_5.detectChangesInNestedViews();
  }

  @override
  void destroyInternal() {
    _appEl_2?.destroyNestedViews();
    _appEl_3?.destroyNestedViews();
  }
}

[leonsenft]

I think the embedded ViewTestViewContainer{2|4} are what we want to look at (these are the ones created by NgIf).

[matanlurey]

Oh. Ok. Let's post those too:

class _ViewTestContainerComponent2 extends AppView<import1.TestContainerComponent> {
  ViewContainer _appEl_0;
  _ViewTestContainerComponent2(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewTestContainerComponent0._renderType;
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_7 = TemplateRef(_appEl_0, viewFactory_TestContainerComponent3);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _appEl_0.detectChangesInNestedViews();
  }
}

class _ViewTestContainerComponent4 extends AppView<import1.TestContainerComponent> {
  import7.DivElement _el_0;
  ViewContainer _appEl_1;
  _ViewTestContainerComponent4(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewTestContainerComponent0._renderType;
  }
  @override
  ComponentRef<import1.TestContainerComponent> build() {
    var doc = import7.document;
    _el_0 = doc.createElement('div');
    final _anchor_1 = createViewContainerAnchor();
    _el_0.append(_anchor_1);
    _appEl_1 = ViewContainer(1, 0, this, _anchor_1);
    TemplateRef _TemplateRef_1_7 = TemplateRef(_appEl_1, viewFactory_TestContainerComponent5);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_1, _TemplateRef_1_7);
    init0(_el_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _appEl_1.detectChangesInNestedViews();
  }
}

[matanlurey]

Ok, I dug into this more after fixing #1558. Indeed only @deferred fails, not nested *ngIfs.

loadDeferred() is implemented like this:

  Future<void> loadDeferred(
    Future loadComponentFunction(),
    Future loadTemplateLibFunction(),
    ViewContainer viewContainer,
    TemplateRef templateRef, [
    void initializer(),
  ]) {
    return Future.wait([loadComponentFunction(), loadTemplateLibFunction()])
        .then((_) {
      if (initializer != null) {
        initializer();
      }
      viewContainer.createEmbeddedView(templateRef);
      viewContainer.detectChangesInNestedViews();
    });
  }

... which is similar enough to NgIf. <ViewContainer>.clear() is implemented as:

  /// Destroys all Views in this container.
  @override
  void clear() {
    for (var i = this.length - 1; i >= 0; i--) {
      remove(i);
    }
  }

... is it possible somehow this doesn't destroy child view containers? I will keep looking.

[matanlurey]

Ok, I think I found it. void destroyInternal() is not implemented for the @deferred template:

class _ViewNestedDeferredTest1 extends AppView<import1.NestedDeferredTest> {
  ViewContainer _appEl_0;
  _ViewNestedDeferredTest1(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewNestedDeferredTest0._renderType;
  }
  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_7 = TemplateRef(_appEl_0, viewFactory_NestedDeferredTest2);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _appEl_0.detectChangesInNestedViews();
  }
}

Contrast with *ngIf:

class ViewNestedDeferredTest0 extends AppView<import1.NestedDeferredTest> {
  ViewContainer _appEl_0;
  NgIf _NgIf_0_9;
  static RenderComponentType _renderType;
  ViewNestedDeferredTest0(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.component, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    rootEl = import7.document.createElement('nested-deferred-test');
    _renderType ??= import8.appViewUtils.createRenderType((import9.isDevMode ? 'asset:_tests/test/regression/1539_nested_template_test.dart' : null), ViewEncapsulation.None, styles$NestedDeferredTest);
    setupComponentType(_renderType);
  }
  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    final _rootEl = rootEl;
    final import7.HtmlElement parentRenderNode = initViewRoot(_rootEl);
    final _anchor_0 = createViewContainerAnchor();
    parentRenderNode.append(_anchor_0);
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_8 = TemplateRef(_appEl_0, viewFactory_NestedDeferredTest1);
    _NgIf_0_9 = NgIf(_appEl_0, _TemplateRef_0_8);
    init(const [], null);
    return null;
  }

  @override
  void detectChangesInternal() {
    final import1.NestedDeferredTest _ctx = ctx;
    _NgIf_0_9.ngIf = _ctx.showOuter;
    _appEl_0.detectChangesInNestedViews();
  }

  @override
  void destroyInternal() {
    _appEl_0?.destroyNestedViews();
  }
}

[leonsenft]

Nice find! That definitely seems like it!

[matanlurey]

Filed #1561 for future refactoring. Will fix for now.

[matanlurey]

... sadly that does not fix it. Will keep investigating. Here is the code after my change:

class _ViewNestedDeferredTest1 extends AppView<import1.NestedDeferredTest> {
  ViewContainer _appEl_0;
  _ViewNestedDeferredTest1(AppView<dynamic> parentView, int parentIndex) : super(import5.ViewType.embedded, {}, parentView, parentIndex, ChangeDetectionStrategy.CheckAlways) {
    componentType = ViewNestedDeferredTest0._renderType;
  }
  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_7 = TemplateRef(_appEl_0, viewFactory_NestedDeferredTest2);
    loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

  @override
  void detectChangesInternal() {
    _appEl_0.detectChangesInNestedViews();
  }

  @override
  void destroyInternal() {
    _appEl_0?.destroyNestedViews();
  }
}

[leonsenft]

Really? That's pretty surprising.

Is it possible that _appEl_0.destroyNestedViews() is called before viewContainer.createEmbeddedView(templateRef) in loadDeferred due to some async timing in your test case?

Even if it's not, how do we actually defend against this case? There's no way to tell if a ViewContainer is a attached or not right?

[matanlurey]

... and its a combination of the above and a race condition: loadDeferred() never checked if the current view was destroyed when creating an embedded view. If I change loadDeferred() to:

  Future<void> loadDeferred(
    Future loadComponentFunction(),
    Future loadTemplateLibFunction(),
    ViewContainer viewContainer,
    TemplateRef templateRef, [
    void initializer(),
  ]) {
    return Future.wait([loadComponentFunction(), loadTemplateLibFunction()])
        .then((_) {
      if (viewData.destroyed) {
        return;
      }
      if (initializer != null) {
        initializer();
      }
      viewContainer.createEmbeddedView(templateRef);
      viewContainer.detectChangesInNestedViews();
    });
  }

EDIT: We came to the same conclusion at the same time :)

[matanlurey]

Oops, you are right. There is no such thing as detecting a detached ViewContainer :(

[leonsenft]

Ironic considering the topic being a race condition! 😆

[leonsenft]

Yeah, we either need a way of detecting if the ViewContainer is detached (doesn't seem likely), or a signal to provided the defer load code not to embed the view. This is pretty wacky, but something like this could work:

  void Function() loadDeferred(
    Future loadComponentFunction(),
    Future loadTemplateLibFunction(),
    ViewContainer viewContainer,
    TemplateRef templateRef, [
    void initializer(),
  ]) {
    var canceled = false;
    Future.wait([loadComponentFunction(), loadTemplateLibFunction()])
        .then((_) {
      if (viewData.destroyed) {
        return;
      }
      if (initializer != null) {
        initializer();
      }
      if (!canceled) {
        viewContainer.createEmbeddedView(templateRef);
        viewContainer.detectChangesInNestedViews();
      }
    });
    return () {
      canceled = true;
    };
  }

  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    ...
    _cancel0 = loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

  @override
  void destroyInternal() {
    _cancel0();
    _appEl_0?.destroyNestedViews();
  }

[leonsenft]

A cancelable future would be even better... but we don't have that. 😄

[matanlurey]

I think your idea is the best one I can think of without adding too much plumbing.

[leonsenft]

Wait a second, what's that viewData.destroyed? I'm assuming that should be set if the view container has been destroyed no? Or is that checking if the hosting component is destroyed?

[matanlurey]

Hosting component :(. Its very wonky, only components can be destroyed, not embedded templates.

[leonsenft]

Alright, well then I realized the canceled check can be hoisted above everything (not just the embedding calls):

  void Function() loadDeferred(
    Future loadComponentFunction(),
    Future loadTemplateLibFunction(),
    ViewContainer viewContainer,
    TemplateRef templateRef, [
    void initializer(),
  ]) {
    var canceled = false;
    Future.wait([loadComponentFunction(), loadTemplateLibFunction()])
        .then((_) {
      if (canceled || viewData.destroyed) {
        return;
      }
      if (initializer != null) {
        initializer();
      }
      viewContainer.createEmbeddedView(templateRef);
      viewContainer.detectChangesInNestedViews();
      }
    });
    return () {
      canceled = true;
    };
  }

[matanlurey]

Agreed. I don't think we need viewData.destroyed though. I almost have a CL now :)

[matanlurey]

... and still fails :( (the test).

I think we should keep this regardless (for correctness), but the hunt continues.

[matanlurey]

I was still not able to reproduce outside of loadDeferred():

@Component(
  selector: 'nested-custom-test',
  directives: [
    CustomIfDirective,
    HelloWorldComponent,
  ],
  template: r'''
    <template customIf #showOuter>
      <template customIf #showInner>
        <hello-world></hello-world>
      </template>
    </template>
  ''',
)
class NestedCustomTest {
  @ViewChild('showOuter', read: CustomIfDirective)
  CustomIfDirective showOuter;

  @ViewChild('showInner', read: CustomIfDirective)
  CustomIfDirective showInner;
}

  test('should render a nested template with a custom directive', () async {
    final fixture = await NgTestBed.forComponent(
      ng.NestedCustomTestNgFactory,
    ).create();

    Future<void> setInnerCondition(bool value) {
      return fixture.update((c) {
        if (value) {
          c.showInner.createChildView();
        } else {
          c.showInner.destroyChildView();
        }
      });
    }

    Future<void> setOuterCondition(bool value) {
      return fixture.update((c) {
        if (value) {
          c.showOuter.createChildView();
        } else {
          c.showOuter.destroyChildView();
        }
      });
    }

    void expectHelloWorld() {
      expect(fixture.text, contains('Hello World'));
    }

    void expectEmpty() {
      expect(fixture.text, isNot(contains('Hello World')));
    }

    expectEmpty();
    await setOuterCondition(true);
    expectEmpty();
    await setInnerCondition(true);
    expectHelloWorld();
    await setOuterCondition(false);
    expectEmpty();
  });

---

Even with this custom directive, everything seems to work properly.

[matanlurey]

Lots more investigating, going back to why the <div *ngIf> case works, and this doesn't.

What actually removes the DOM is:

  void detachViewNodes(List<Node> viewRootNodes) {
    _detachAll(viewRootNodes);
  }

... which in turn is called by <ViewContainer>.detachView:

  AppView detachView(int viewIndex) {
    var view = nestedViews.removeAt(viewIndex);
    if (view.viewData.type == ViewType.component) {
      throw StateError("Component views can't be moved!");
    }
    view.detachViewNodes(view.flatRootNodes);
    if (view.inlinedNodes != null) {
      view.detachViewNodes(view.inlinedNodes);
    }
    view.removeFromContentChildren(this);
    return view;
  }

---

So I looked into how the template itself was anchored. Here is with the <div>:

  @override
  ComponentRef<import1.NestedDeferredWithDivTest> build() {
    var doc = import7.document;
    _el_0 = doc.createElement('div');
    final _anchor_1 = createViewContainerAnchor();
    _el_0.append(_anchor_1);
    _appEl_1 = ViewContainer(1, 0, this, _anchor_1);
    TemplateRef _TemplateRef_1_7 = TemplateRef(_appEl_1, viewFactory_NestedDeferredWithDivTest2);
    _cancelDeferredLoad1 = loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_1, _TemplateRef_1_7);
    init0(_el_0);
    return null;
  }

... and here is without:

  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_7 = TemplateRef(_appEl_0, viewFactory_NestedDeferredTest2);
    _cancelDeferredLoad0 = loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

... so I am going to try and track down what init0 does, and how that eventually is cleaned up.

[matanlurey]

Here is the DOM in a minimal sample before the initial *ngIf=true:

<hello-world>
  <button>Toggle</button>
  <!---->
</hello-world>

Initial:

<hello-world>
  <button>Toggle</button>
  <!---->
  <!---->
  <i-am-buggy>
    BUG BUG BUG
  </i-am-buggy>
</hello-world>

And toggled off:

<hello-world>
  <button>Toggle</button>
  <!---->
  <i-am-buggy>
    BUG BUG BUG
  </i-am-buggy>
</hello-world>

I'm guessing this "just works" with a <div> because it allows child elements. I was confused how the nested *ngIf works though (without a <div>), so I wrote a sample:

Before:

<div><button>Toggle</button><!----></div>

After:

<div><button>Toggle</button><!----><!----><i-am-buggy>BUG BUG BUG</i-am-buggy></div>

Toggled off:

<div><button>Toggle</button><!----></div>

So still, something subtly different for @deferred compared to <template> :(

[matanlurey]

Ok, I now wonder if the indices are incorrect in the @deferred case.

With a <div>:

//       this.index, this.parentIndex
_appEl_1 = ViewContainer(1, 0, this, _anchor_1);

Without:

//       this.index, this.parentIndex
_appEl_0 = ViewContainer(0, null, this, _anchor_0);

The second null seems suspect.

[matanlurey]

Ok, that null is triggered in this case:

    if ((hasViewContainer || hasEmbeddedView) && !isInlined) {
      appViewContainer = view.createViewContainer(
        renderNode,
        nodeIndex,
        !hasViewContainer,
        isRootElement ? null : parent.nodeIndex,
      );
      _providers.add(Identifiers.ViewContainerToken, appViewContainer);
    }

I'm guessing isRootElement is triggering true for deferred templates for some reason:

  /// Whether node is the root of the view.
  bool get isRootElement => view != parent.view;

(I have no idea why the above is the condition for being a root element, btw)

[matanlurey]

If I always emit parent.nodeIndex, it is still null:

_appEl_0 = ViewContainer(0, null, this, _anchor_0);

... so isRootElement is unlikely the issue, we are storing null for parent.nodeIndex.

[matanlurey]

That might be a red herring, it looks like it can be null and work fine in other cases:

  @override
  ComponentRef<import1.NestedTemplateTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_8 = TemplateRef(_appEl_0, viewFactory_NestedTemplateTest2);
    _NgIf_0_9 = NgIf(_appEl_0, _TemplateRef_0_8);
    init0(_appEl_0);
    return null;
  }

[matanlurey]

One thing I had not checked at this point is if the DOM is stale (i.e. just a remanent) or live.

So I added some code that ran a Timer and updated the deferred component:

@Component(
  selector: 'i-am-buggy',
  template: 'BUG BUG BUG {{counter}}',
)
class BugComponent {
  var counter = 0;
  
  BugComponent() {
    new Timer.periodic((const Duration(seconds: 1)), (_) => counter++);
  }
}

... it's dead views, so its really just the root DOM node that is never getting cleaned up.

[matanlurey]

@leonsenft suggested adding a breakpoint on the removal, so I added one here:

void _detachAll(List<Node> viewRootNodes) {
  int len = viewRootNodes.length;
  for (var i = 0; i < len; i++) {
    Node node = viewRootNodes[i];
    node.remove();
    domRootRendererIsDirty = true;
  }
}

For the working case, it is called with 2 nodes, [comment, i-am-buggy].
For the buggy case, it is only called with a single node, [comment].

... which reflects the behavior I'm seeing with i-am-buggy never being removed from the DOM.

[matanlurey]

Adding:

  void detachViewNodes(List<Node> viewRootNodes) {
    print('>>> $this.detachViewNodes($viewRootNodes)');
    _detachAll(viewRootNodes);
  }

... prints ...

// Working
>>> Instance of '_ViewHelloWorldComponent3'.detachViewNodes([, i-am-buggy])

// Buggy
>>> Instance of '_ViewHelloWorldComponent1'.detachViewNodes([])

[matanlurey]

And that eventually comes from view.detachViewNodes(view.flatRootNodes)... ugh.

So, it makes me think this is buggy in some way:

  List<Node> get flatRootNodes =>
      _flattenNestedViews(viewData.rootNodesOrViewContainers);

... but I'll get more data using:

List<Node> _flattenNestedViews(List nodes) {
  print('>>> _flattenNestedViews(${nodes.length}: $nodes)');
  return _flattenNestedViewRenderNodes(nodes, <Node>[]);
}

... prints ...

// Working
>>> _flattenNestedViews(1: [Instance of 'ViewContainer'])
>>> _flattenNestedViews(1: [i-am-buggy])
>>> _flattenNestedViews(1: [Instance of 'ViewContainer'])
>>> Instance of '_ViewHelloWorldComponent3'.detachViewNodes([, i-am-buggy])

// Buggy
>>> _flattenNestedViews(1: [])
>>> _flattenNestedViews(1: [i-am-buggy])
>>> _flattenNestedViews(1: [])

... so the first call to this function is missing the view container in the deferred case.

[matanlurey]

... and found it:

// Working
  @override
  ComponentRef<import1.NestedDeferredWithDivTest> build() {
    var doc = import7.document;
    _el_0 = doc.createElement('div');
    final _anchor_1 = createViewContainerAnchor();
    _el_0.append(_anchor_1);
    _appEl_1 = ViewContainer(1, 0, this, _anchor_1);
    TemplateRef _TemplateRef_1_7 = TemplateRef(_appEl_1, viewFactory_NestedDeferredWithDivTest2);
    _cancelDeferredLoad1 = loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_1, _TemplateRef_1_7);
    init0(_el_0);
    return null;
  }

// Buggy
  @override
  ComponentRef<import1.NestedDeferredTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_7 = TemplateRef(_appEl_0, viewFactory_NestedDeferredTest2);
    _cancelDeferredLoad0 = loadDeferred(deflib0.loadLibrary, deflib1.loadLibrary, _appEl_0, _TemplateRef_0_7);
    init0(_anchor_0);
    return null;
  }

Notice the difference?

In // Working, init0 stores the <div> element, which contains <i-am-buggy> (eventually). It does not contain _appEl_1, but that is OK sort of accidentally, because _el_0 has all of the child nodes.

In // Buggy, init0 stores only the _anchor_0, which is not enough information to remove the DOM of the child view. It should be storing init0(_appEl_0).

When you use nested template tags manually (not @deferred) it writes:

  @override
  ComponentRef<import1.NestedTemplateTest> build() {
    final _anchor_0 = createViewContainerAnchor();
    _appEl_0 = ViewContainer(0, null, this, _anchor_0);
    TemplateRef _TemplateRef_0_8 = TemplateRef(_appEl_0, viewFactory_NestedTemplateTest2);
    _NgIf_0_9 = NgIf(_appEl_0, _TemplateRef_0_8);
    init0(_appEl_0);
    return null;
  }

Which is correct, _appEl_0 is what is significant here, not the anchor.

[matanlurey]

OK, so this is due to the fact that <template @deferred> (the synthetic node, you can't write this yourself) is the only example of a <template> element that doesn't seem to interact with ViewContainerRef (NgIf does, for example), so the compiler hits an "optimization" case where hasViewContainer: false - and it is not stored as a root element.

Deferred views should insist they need the view container.

[matanlurey]

Closed in 47f948a.