Skip to content
This repository was archived by the owner on Sep 16, 2022. It is now read-only.

Intent to remove: QueryList #688

Closed
matanlurey opened this issue Nov 13, 2017 · 5 comments
Closed

Intent to remove: QueryList #688

matanlurey opened this issue Nov 13, 2017 · 5 comments

Comments

@matanlurey
Copy link
Contributor

The class QueryList is a narrow interface on top of List with a .changes getter:

class Comp implements AfterViewInit {
  @ViewChildren(ChildDirective)
  QueryList<ChildDirective> children;

  @override
  ngAfterViewInit() {
    children.listen((_) { ... });
  }
}

However, this pattern isn't used widely, and the changes stream just returns all of the items in the list anyway (no diffing). It would be less costly and more idiomatic with the rest of the framework to just support a setter:

class Comp {
  @ViewChildren(ChildDirective)
  set children(List<Directive> directives) {
    // List of directives has changed.
  }
}

It would also remove the code needed to support QueryList, and the delegating calls to List.

@matanlurey
Copy link
Contributor Author

(The plan in my head would be to support the setter syntax, migrate all users to the setter syntax, and then remove the QueryList class. It would require relatively small changes in both the runtime and the compiler.)

alorenzen pushed a commit that referenced this issue Nov 20, 2017
The rationale for this work is mostly to be able to share tests for the
upcoming query simplification (#688)
- I'll be adding the rest of the test cases here before proceeding.

PiperOrigin-RevId: 176153248
alorenzen pushed a commit that referenced this issue Nov 20, 2017
The rationale for this work is mostly to be able to share tests for the
upcoming query simplification (#688)
- I'll be adding the rest of the test cases here before proceeding.

PiperOrigin-RevId: 176153248
alorenzen pushed a commit that referenced this issue Nov 20, 2017
The rationale for this work is mostly to be able to share tests for the
upcoming query simplification (#688)
- I'll be adding the rest of the test cases here before proceeding.

PiperOrigin-RevId: 176153248
alorenzen pushed a commit that referenced this issue Nov 21, 2017
The rationale for this work is mostly to be able to share tests for the
upcoming query simplification (#688)
- I'll be adding the rest of the test cases here before proceeding.

PiperOrigin-RevId: 176153248
alorenzen pushed a commit that referenced this issue Nov 21, 2017
The rationale for this work is mostly to be able to share tests for the
upcoming query simplification (#688)
- I'll be adding the rest of the test cases here before proceeding.

PiperOrigin-RevId: 176153248
@matanlurey
Copy link
Contributor Author

matanlurey commented Nov 27, 2017

This change has been accepted, and will be started.

The plan currently is:

  • Add no-op support for a field typed with dart:core#List.
  • Add actual implementation of the new query list.
  • Deprecate QueryList.
  • Allow the field to be typed Iterable as well as List for the new functionality.
  • Migrate existing use cases to dart:core#List.
  • Remove QueryList.

I expect this to take a decent amount of time, hopefully complete by v5.

@matanlurey
Copy link
Contributor Author

Design

Currently the following code is generated to support QueryList:

// query.dart
import 'package:angular/angular.dart';

@Component(
  selector: 'example',
  directives: const [ChildDirective, NgIf],
  template: r'''
    <child *ngIf="someValue"></child>
    <child></child>
  ''',
)
class QueryExample {
  @ViewChildren(ChildDirective)
  QueryList<ChildDirective> childList;
}

@Directive(selector: 'child')
class ChildDirective {}
// query.template.dart
class ViewQueryExample0 extends AppView<import1.QueryExample> {
  import2.QueryList _viewQuery_ChildDirective_0;
  ViewContainer _appEl_0;
  import1.ChildDirective _ChildDirective_0_4;
  import1.ChildDirective _ChildDirective_1_4;

  @override
  ComponentRef<import1.QueryExample> build() {
    _viewQuery_ChildDirective_0 = new import2.QueryList();
    _ChildDirective_0_4 = new import1.ChildDirective();
    _ChildDirective_1_4 = new import1.ChildDirective();
  }

 @override
  void detectChangesInternal() {
    if (_viewQuery_ChildDirective_0.dirty) {
      _viewQuery_ChildDirective_0.reset([
        _appEl_0.mapNestedViews(_ViewQueryExample1, (nestedView) {
          return [nestedView._ChildDirective_0_4];
        }),
        _ChildDirective_1_4
      ]);
      ctx.childList = _viewQuery_ChildDirective_0;
      _viewQuery_ChildDirective_0.notifyOnChanges();
    }
  }
}

class _ViewQueryExample1 extends AppView<import1.QueryExample> {
 @override
  void dirtyParentQueriesInternal() {
    (parentView as ViewQueryExample0)._viewQuery_ChildDirective_0.setDirty();
  }
}
// query_proposal.dart
import 'package:angular/angular.dart';

@Component(
  selector: 'example',
  directives: const [ChildDirective, NgIf],
  template: r'''
    <child *ngIf="someValue"></child>
    <child></child>
  ''',
)
class QueryExample {
  @ViewChildren(ChildDirective)
  List<ChildDirective> childList;
}

@Directive(selector: 'child')
class ChildDirective {}
// query_proposal.template.dart
class ViewQueryExample0 extends AppView<import1.QueryExample> {
  bool _viewQuery_ChildDirective_0isDirty = true;
  ViewContainer _appEl_0;
  import1.ChildDirective _ChildDirective_0_4;
  import1.ChildDirective _ChildDirective_1_4;

  @override
  ComponentRef<import1.QueryExample> build() {
    _ChildDirective_0_4 = new import1.ChildDirective();
    _ChildDirective_1_4 = new import1.ChildDirective();
  }

 @override
  void detectChangesInternal() {
    if (_viewQuery_ChildDirective_0isDirty) {
      ctx.childList = someHelper([
        _appEl_0.mapNestedViews(_ViewQueryExample1, (nestedView) {
          return [nestedView._ChildDirective_0_4];
        }),
        _ChildDirective_1_4
      ]);
      _viewQuery_ChildDirective_0isDirty = false;
    }
  }
}

class _ViewQueryExample1 extends AppView<import1.QueryExample> {
 @override
  void dirtyParentQueriesInternal() {
    (parentView as ViewQueryExample0)._viewQuery_ChildDirective_0isDirty = true;
  }
}

Obviously this code could still use further optimizations, but the simplification of just setting immutable lists of child directives/elements/etc (and removed wrapping of QueryList, which Dart2JS knows isn't a JSArray) is an improvement overall and can lead to further improvements.

alorenzen pushed a commit that referenced this issue Dec 1, 2017
Implements legacy support for #688.

Reverted the change that split values => values, templates. Apparently the order is significant, so it's important (right now, at least) not to split the two into separate fields.

Major change is QueryList's are eagerly created as final class members. This should let Dart2JS reason about them slightly better, i.e. they can see they are never mutated. Otherwise no impact to timing or user code.

PiperOrigin-RevId: 177518597
alorenzen pushed a commit that referenced this issue Dec 1, 2017
See design for details:
#688 (comment)

* Cleaned up CompileQueryMetadata to remove unused fields, use finals.
* Added `propertyType` as a new field on CompileQueryMetadata.
* Simplifies the act of flattening List<List<T>> into a single method.

This was necessary because otherwise it's impossible to use type inference.
The overhead should be minimal, it is an extra List allocation for elements
that are not mapped, but once mapping is used the cost is high anyway.

Here is a typical "dynamic" query with the new List format:
    ctx.viewChildren = import12.flattenNodes([
        [_AnotherDirective_0_4],
        _appEl_1.mapNestedViews((_ViewEmbeddedQueriesList1 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        }),
        _appEl_2.mapNestedViews((_ViewEmbeddedQueriesList2 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        })
      ]);

Here is a typical "static" query with the new List format:
    ctx.actualChildren = [
      _ValueDirective_0_4,
      _ValueDirective_1_4,
      _ValueDirective_2_4,
    ];

PiperOrigin-RevId: 177544954
alorenzen pushed a commit that referenced this issue Dec 1, 2017
Implements legacy support for #688.

Reverted the change that split values => values, templates. Apparently the order is significant, so it's important (right now, at least) not to split the two into separate fields.

Major change is QueryList's are eagerly created as final class members. This should let Dart2JS reason about them slightly better, i.e. they can see they are never mutated. Otherwise no impact to timing or user code.

PiperOrigin-RevId: 177518597
alorenzen pushed a commit that referenced this issue Dec 1, 2017
See design for details:
#688 (comment)

* Cleaned up CompileQueryMetadata to remove unused fields, use finals.
* Added `propertyType` as a new field on CompileQueryMetadata.
* Simplifies the act of flattening List<List<T>> into a single method.

This was necessary because otherwise it's impossible to use type inference.
The overhead should be minimal, it is an extra List allocation for elements
that are not mapped, but once mapping is used the cost is high anyway.

Here is a typical "dynamic" query with the new List format:
    ctx.viewChildren = import12.flattenNodes([
        [_AnotherDirective_0_4],
        _appEl_1.mapNestedViews((_ViewEmbeddedQueriesList1 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        }),
        _appEl_2.mapNestedViews((_ViewEmbeddedQueriesList2 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        })
      ]);

Here is a typical "static" query with the new List format:
    ctx.actualChildren = [
      _ValueDirective_0_4,
      _ValueDirective_1_4,
      _ValueDirective_2_4,
    ];

PiperOrigin-RevId: 177544954
alorenzen pushed a commit that referenced this issue Dec 1, 2017
Implements legacy support for #688.

Reverted the change that split values => values, templates. Apparently the order is significant, so it's important (right now, at least) not to split the two into separate fields.

Major change is QueryList's are eagerly created as final class members. This should let Dart2JS reason about them slightly better, i.e. they can see they are never mutated. Otherwise no impact to timing or user code.

PiperOrigin-RevId: 177518597
alorenzen pushed a commit that referenced this issue Dec 1, 2017
See design for details:
#688 (comment)

* Cleaned up CompileQueryMetadata to remove unused fields, use finals.
* Added `propertyType` as a new field on CompileQueryMetadata.
* Simplifies the act of flattening List<List<T>> into a single method.

This was necessary because otherwise it's impossible to use type inference.
The overhead should be minimal, it is an extra List allocation for elements
that are not mapped, but once mapping is used the cost is high anyway.

Here is a typical "dynamic" query with the new List format:
    ctx.viewChildren = import12.flattenNodes([
        [_AnotherDirective_0_4],
        _appEl_1.mapNestedViews((_ViewEmbeddedQueriesList1 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        }),
        _appEl_2.mapNestedViews((_ViewEmbeddedQueriesList2 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        })
      ]);

Here is a typical "static" query with the new List format:
    ctx.actualChildren = [
      _ValueDirective_0_4,
      _ValueDirective_1_4,
      _ValueDirective_2_4,
    ];

PiperOrigin-RevId: 177544954
alorenzen pushed a commit that referenced this issue Dec 1, 2017
Implements legacy support for #688.

Reverted the change that split values => values, templates. Apparently the order is significant, so it's important (right now, at least) not to split the two into separate fields.

Major change is QueryList's are eagerly created as final class members. This should let Dart2JS reason about them slightly better, i.e. they can see they are never mutated. Otherwise no impact to timing or user code.

PiperOrigin-RevId: 177518597
alorenzen pushed a commit that referenced this issue Dec 1, 2017
See design for details:
#688 (comment)

* Cleaned up CompileQueryMetadata to remove unused fields, use finals.
* Added `propertyType` as a new field on CompileQueryMetadata.
* Simplifies the act of flattening List<List<T>> into a single method.

This was necessary because otherwise it's impossible to use type inference.
The overhead should be minimal, it is an extra List allocation for elements
that are not mapped, but once mapping is used the cost is high anyway.

Here is a typical "dynamic" query with the new List format:
    ctx.viewChildren = import12.flattenNodes([
        [_AnotherDirective_0_4],
        _appEl_1.mapNestedViews((_ViewEmbeddedQueriesList1 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        }),
        _appEl_2.mapNestedViews((_ViewEmbeddedQueriesList2 nestedView) {
          return [nestedView._AnotherDirective_0_4];
        })
      ]);

Here is a typical "static" query with the new List format:
    ctx.actualChildren = [
      _ValueDirective_0_4,
      _ValueDirective_1_4,
      _ValueDirective_2_4,
    ];

PiperOrigin-RevId: 177544954
@matanlurey matanlurey changed the title Intent to deprecate: QueryList Intent to remove: QueryList Jan 26, 2018
@matanlurey
Copy link
Contributor Author

This is now considered deprecated, pending removal before 5.0.0-beta.

@matanlurey
Copy link
Contributor Author

Done at HEAD.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.