Angular: Structural directives that use the ComponentLoader don't work with @ViewChild

[enyo]

Dart VM version: 2.0.0-dev.63.0 (Fri Jun 15 00:42:43 2018 +0200) on "macos_x64"
Angular 5.0.0-alpha+15
macOS

I have create a demo repo to isolate the issue.

Short explanation

If a component has this HTML:

<div *myStructural="true">
  <div #foo></div>
</div>

and uses @ViewChild('foo') to hold a reference to the div, everything works fine, until the MyStructuralDirective replaces the content with:

    _viewContainer.clear();
    _loader.loadNextTo(ng.LoadingComponentNgFactory);

(So the whole content of the div is replaced with the LoadingComponent)

In this case the following exception is thrown:

EXCEPTION: Type '_ViewLoadingComponentHost0' is not a subtype of expected type '_ViewAppComponent1'.

In our project, this structural directive is used to show a loading component when something is not yet loaded, and only create the actual content when loading is finished.

[leonsenft]

The issue here is that child queries are static. Code is generated at compile-time with assumptions about the structure of your DOM and location of #-references.

For example, here's a small excerpt from the code generated for setting the ViewChild query in your example:

_appEl_0.mapNestedViews((_ViewAppComponent1 nestedView) {
  return [nestedView._el_2];
})

This is the code that performs the lookup for the #foo reference. Let's break this down so it makes sense.

• _ViewAppComponent1 is a generated view that represents the DOM embedded by the structural directive. This includes the outer and inner divs.
• _el_2 is the second element in this embedded view. This is the div that was marked with #foo.

The advantage of this design is that no expensive query logic is needed at runtime. You can see no mention of #foo was needed at all. However, as you've discovered this optimization has a tradeoff: queries don't work with dynamically generated DOM.

Now there's no problem with dynamically removing and reinserting the same DOM structure like NgIf does. So long as the reinserted view has the expected type (_ViewAppComponent1 in the above case) the query will work.

What's happening in your case, is you've removed the _ViewAppComponent1, and replaced it with the generated host view of your dynamically loaded LoadingComponent. This isn't the expected view type, and thus you get the type error when we implicitly cast the view to its expected type here:

https://github.com/dart-lang/angular/blob/0b7878f81b8441e290e436a0f651fbe1e667e522/angular/lib/src/core/linker/view_container.dart#L169-L171

I can think of two possible resolutions off the top of my head:

1. We mark this as working as intended, and preserve the current restriction that child queries don't support this use case.

2. We concede some performance and change the code in mapNestedViews to check if the nested view has the expected type. This would not make any dynamically inserted references queryable, but it would prevent the generated code from throwing a type error in this use case.

@matanlurey has this issue come up before? Have we considered any other solutions or accepted this limitation?

[matanlurey]

This is an interesting one, @enyo thanks for filing!

@leonsenft:

(As far as I know, this _hasn't_ come up before. @TedSander have you seen this internally?)

The issue here is that child queries are static. Code is generated at compile-time with assumptions about the structure of your DOM and location of #-references.

Correct. Similar to dynamically inserting DOM other ways (i.e. with dart:html).

For example, here's a small excerpt from the code generated for setting the ViewChild query in your example:

_appEl_0.mapNestedViews((_ViewAppComponent1 nestedView) {
  return [nestedView._el_2];
})

This is the code that performs the lookup for the #foo reference. Let's break this down so it makes sense.

• _ViewAppComponent1 is a generated view that represents the DOM embedded by the structural directive. This includes the outer and inner divs.
• _el_2 is the second element in this embedded view. This is the div that was marked with #foo.

The advantage of this design is that no expensive query logic is needed at runtime. You can see no mention of #foo was needed at all. However, as you've discovered this optimization has a tradeoff: queries don't work with dynamically generated DOM.

Now there's no problem with dynamically removing and reinserting the same DOM structure like NgIf does. So long as the reinserted view has the expected type (_ViewAppComponent1 in the above case) the query will work.

And also explains why users haven't run into this (as far as I know): custom structural directives are rare, and ones that swap content _and_ use a content or view query are probably even rarer, hence this bug being only hit now.

What's happening in your case, is you've removed the _ViewAppComponent1, and replaced it with the generated host view of your dynamically loaded LoadingComponent. This isn't the expected view type, and thus you get the type error when we implicitly cast the view to its expected type here:

https://github.com/dart-lang/angular/blob/0b7878f81b8441e290e436a0f651fbe1e667e522/angular/lib/src/core/linker/view_container.dart#L169-L171

I can think of two possible resolutions off the top of my head:

1. We mark this as working as intended, and preserve the current restriction that child queries don't support this use case.

And ideally update the documentation on ComponentLoader (and friends) to warn of this case.

1. We concede some performance and change the code in mapNestedViews to check if the nested view has the expected type. This would not make any dynamically inserted references queryable, but it would prevent the generated code from throwing a type error in this use case.

We could theoretically use unsafeCast instead of typing the closures, but I'm worried if some of the functionality isn't _actually_ supported then we aren't really doing anybody any favors (and make the generated code more complex as a result).

A third option would be change (again) how we store this information. For example, imagine:

abstract class EmbeddedAppView {
  List<Element> get domElements;
}


// We no longer need strict typing to refer to `._el_2`.
_appEl_0.mapNestedViews((EmbeddedAppView embeddedView) {
  return [embeddedView.domElements[1]];
});

We _might_ want to go down this route at some point for code size _anyway_?

There is a fourth option, which is support dynamic querying (opt-in). I don't think its realistic and I'd want to see such a feature request come from multiple parties/partners first (I haven't seen this come up yet).

---

@enyo: A few workarounds I can think of:

1. A pattern similar to DeferredContentDirective and DeferredContentAware - basically have your structural directive inject some sort of interface and pass itself to the parent:

   class ChildStructuralDirective {
   ChildStructuralDirective(ParentComponent c) {
     c.onChild(this);
   }
   }
   

2. Have your ChildStructuralDirective have an @Output() that emits itself:

   class ChildStructuralDirective {
   @Output()
   Stream<ChildStructuralDirective> get onActive => new Stream.fromIterable([this]);
   }
   

[leonsenft]

@matanlurey While I like the idea of storing DOM elements generally in a list rather than as fields, this would actually enable a new bug. Swapping in a new embedded view would then not crash, but you'd potentially return an unintended DOM element that wasn't marked with the same reference, but happened to have the same index.

[matanlurey]

True!

[TedSander]

Nope I haven't seen this before for the reasons you outlined.

[matanlurey]

Thanks. I'm guessing we should close as WAI though I'm supportive of updating docs for both the query annotations and component loading to clarify what is supported.

[matanlurey]

I'll handle this in https://github.com/dart-lang/angular/pull/1462.

[enyo]

Thank you all for tour time to review this and respond so thoroughly. The reason why this is happening is now crystal clear.

@matanlurey I’m not quite sure how the directive passing itself to the parent will help in this example. Do you mind elaborating your two examples a bit more?

[matanlurey]

Fixed in #1462.

[enyo]

@matanlurey Ah, I understand your workaround now. I thought you meant that my directive that hides and shows the content should inject itself, but you are talking about the components I want to track with @ViewChild right? So instead of using @ViewChild I can let the component itself inject itself into the parent.