Allow substituting type annotation for @Query(read: ...)

[matanlurey]

For example:

Sponsored by: <template #currentAd></template>

@ViewChild('currentAd', read: ViewContainerRef)
ViewContainerRef currentAd;

Could be simplified to:

@ViewChild('currentAd')
ViewContainerRef currentAd;

I'd love to simplify it further to just @ViewChild(), but that will be a breaking change.

[alorenzen]

If only we could have positional and named params in the same parameter list...

[leonsenft]

I've run into the following issue working on this.

Before, you could query for a base type via reference, and have it match derived types.

@ViewChildren('tab')
QueryList<AbstractTab> tabs;

It's worth noting this works without a read token since a component element with a reference publishes its instance under that reference for querying (similar to TemplateRef on a template element).

If we instead infer the read type, it then queries explicitly for instances of AbstractTab, and fails to match any derived types. This means we can't treat an inferred read value the same as a user specified read value.

The motivating example @matanlurey posted requires that the inferred read token take precedence over the reference selector, whereas the example I've posted above requires the opposite.

I can see two potential solutions, but both require more work than initially anticipated.

1. Augment the compiler to make queries contravariant so that a query for AbstractTab will match all of its derived and implementing types. This could potentially break clients as they could be relying on @ViewChild('foo', read: 'FooDirective') to return exactly only FooDirective and not one of its derived types.
2. When inferring the read token, query first by selector. Only if that fails, default to the inferred read token instead. A query fails if it doesn't match and returns null, or returns an incompatible type. The difficulty here is again determining type compatibility, which would require augmenting the query metadata to track inheritance information.

@matanlurey @alorenzen Thoughts?

[zoechi]

I'd prefer: If no read parameter is passed, try to infer from the field type.
Every not covered case requires the read parameter to be set explicitly.

[leonsenft]

@zoechi The issue is inference doesn't work with inheritance currently, and if you set the read token explicitly it won't work with inheritance for the same reason. Consider this case.

abstract class BaseTab {}

@Component(selector: 'foo-tab', ...)
class FooTab extends BaseTab {}

@Component(selector: 'bar-tab', ...)
class BarTab extends BaseTab {}

@Component(
  template: '''
<foo-tab #tab></foo-tab>
<bar-tab #tab></bar-tab>
''',
  directives: const [FooTab, BarTab],
  ...
)
class TabPanel {
  @ViewChildren('tab')
  QueryList<BaseTab> tabs;
}

Here we can't infer or use read explicitly since in both cases BaseTab won't match its derived types.

I've been doing some more testing and it looks like we may just be able to rely on the following process:

1. If read is provided, use it to match.
2. Otherwise, first query by selector. If that fails, then infer the read type.

I thought this wouldn't work because of the issues I detailed in my last post, but I may have been wrong. Still verifying.

[zoechi]

@leonsenft I think this is basically what I suggested

2. How can the selector fail? I think this should be an error.

[leonsenft]

Fail as in not match anything, as in no instance of the thing you're querying for currently exists in the view. This isn't an error. Consider your query target is inside an ngIf, sometimes the query has a match, and sometimes it won't.

[zoechi]

I assumed the information to be available statically (template variable exists in the template, or component or directive is used) Not getting a result at runtime is a different story and not an error (like your *ngIf example) Components added dynamically using ViewContainerRef aren't found anyway, are they?

I don't know about ContentChildren though. I guess here it's not possible to check statically in every case.

[leonsenft]

Yes, actually my example was runtime specific, but the original issue is about compile-time. Sorry for the bad example, that was confusing.

The issue I'm mentioning is less to do with what's statically possible, and more with the limitations of our compiler's current implementation.

What I meant by the selector failing, is not being able to find the correct type associated with a reference.

For example, if you have the following directive,

@Directive(selector: '[foo]')
class FooDirective {}

and template,

<div foo #ref></div>

and query

@ViewChild('ref')
FooDirective foo;

this will currently fail. There's no association between the FooDirective and any query information. By fail I mean it will fall back to querying for the ElementRef associated with #ref, and will assign the ElementRef to foo at runtime and crash. This of course can be fixed with the introduction of the read token. This is what I meant by the selector failing to match properly.

[zoechi]

So, what should be possible is to

• use the type annotation as default value for read.
• if that is not the desired behavior, the user needs to pass read explicitly.
• if the type annotation is dynamic or object, use the current behavior.

I think this would be fine. I don't see a real benefit in trying to deriving more information from wherever.

[leonsenft]

use the type annotation as default value for read.

We can't do that though. As I've pointed out, you have to give precedence to the query results for a reference selector, before doing any type restriction. Please go back and look at the tabs example for why this is the case.

[leonsenft]

I've found a contradiction which I think makes this implementing this feature with the current API potentially too confusing for users.

Reference Precedes Inference

Querying by reference has to take precedence over querying by inferred read token. Consider this example.

abstract class TabBase {}

@Component(selector: 'foo-tab', ...)
class FooTab extends TabBase {}

@Component(selector: 'bar-tab', ...)
class BarTab extends TabBase {}

@Component(selector: 'tab-panel', ...)
class TabPanel {
  @ContentChildren('tab')
  QueryList<TabBase> tabs;
}

<tab-panel>
  <foo-tab #tab></foo-tab>
  <bar-tab #tab></bar-tab>
</tab-panel>

Here the TabPanel queries for all children with the tab reference, and relies on the fact that a ReferenceAst will be generated for any component host elements with a reference on them. This associates the component instance with that reference for querying.

If we were to instead infer the read token to be TabBase, this example would cease to work, since neither of projected tab components are of type TabBase. The query system has no knowledge of inheritance and won't match FooTab or BarTab when trying to read a TabBase.

Inference Precedes Reference

An issue arises when giving precedence to the reference query when querying for a directive, which is placed on a component's host element.

@Directive(selector: 'bar', ...)
class BarDirective {}

@Component(selector: 'foo', ...)
class FooComponent {
  @ContentChildren('ref')
  BarDirective bar;
}

<foo bar #ref>
</foo>

Here we want to infer a BarDirective but we first must give precedence check for a match from ref. But because ref was placed on a component's host element, we actually get a match for the FooComponent instead. The easy "fix" for this issue is to add an explicit read token for BarDirective.

Conclusion

Adding inference with confusing edges cases isn't worth it. It's not intuitive that a query for a directive by reference works, unless it happens to be on a component host element, in which case the read token is required.

Rather than overload the existing query API and introduce ambiguous use cases, would it be preferable to introduce named constructors for queries with explicit purposes?

// Queries for exact instances of Tab.
@ContentChildren(Tab)
QueryList<Tab> tabs;

// Queries for instance of any type that implements, extends, or mixes-in TabBase.
@ContentChildren.implements(TabBase)
QueryList<TabBase> tabs;

(Breaking) Or perhaps modify the existing constructor to always infer, with more explicit parameters to control inference.

class ContentChildren ... {
  ...
  ContentChildren({
    String reference,
    Type type,
    bool includeSubtypes: true,
  });
}

/// Matches all implementations of `Tab`.
@ContentChildren()
QueryList<Tab> tabs;

/// Matches only implementations of `Tab` with a `tab` reference.
@ContentChildren(reference: 'tab')
QueryList<Tab> tabs;

/// Matches only implementations of `DerivedTab`.
///
/// Normally you could change the `tabs` field instead of using this parameter,
/// but this is useful if you need to restrict the query type of an inherited query.
@ContentChildren(type: DerivedTab)
@override
QueryList<Tab> tabs;

/// Matches only instances of exactly `Tab`.
@ContentChildren(includeSubtypes: false)
QueryList<Tab> tabs;

I'll leave this issue open to gather feedback and assess interest.

[matanlurey]

My intuition is to roll-out slowly, and only support unambiguous inference:

@Component(
  directives: const [Child],
  template: '''
    <child></child>
    <child></child>
  ''',
)
class Comp {
  @view
  List<Child> child;
}

In this case, we clearly know what child is.

[leonsenft]

@matanlurey Does that query for exactly a Child, or any type assignable to Child (such as a Grandchild which extends Child)?

[matanlurey]

Exactly a Child. Keep it simple. We can always expand later.

EDIT: Specifically, this should be sugar for basically saying:

Give me all instances of directive Child in my View (template).

[matanlurey]

@leonsenft: WDUT of my initial proposal? I might be able to tackle this!

[matanlurey]

Note that we technically already support this for List, and soon Element: https://github.com/dart-lang/angular/issues/814

... so it probably makes sense to generalize this soon.

[matanlurey]

Do we want users to write something like:

class MyComp {
  @Children.fromView()
  List<ChildComp> childComps;
}

... instead of ...

class MyComp {
  @ViewChildren(read: ChildComp)
  List<ChildComp> childComps;
}

It's not clear how generics would be handled here, at all.

[Chudesnov]

@matanlurey has something like @ContentChildren.implements(TabBase) made it into the team's plans, or selecting by a base class / interface is unlikely to be supported in AngularDart?