angulardart / angular

Fast and productive web framework provided by Dart
https://pub.dev/packages/angular
MIT License
1.83k stars 233 forks source link

Change detection not working when component loaded with `loadDetached` #1797

Open matthewtsmith opened 5 years ago

matthewtsmith commented 5 years ago

Dart VM version: 2.3.0-dev.0.3 (Tue Apr 23 12:02:59 2019 -0700) on "macos_x64" AngularDart 5.3.0 AngularComponents 1.13.0 OSX Chrome

We have built a VirtualScroll component similar to Android's RecyclerView. Our VirtualScroll component has hooks for the following

  1. Create the child component
  2. Set your data on the component
  3. Clear the component

Here is the code:

  void init() {
    _scroller = new VirtualScroller(
      layout: Layout(),
      container: parentDiv,
      createElement: (idx) {
        if (_pool.isNotEmpty) {
          return _pool.removeLast();
        }
        var ref = _loader.loadDetached(
            ngTemplateContainerTemplate.TemplateContainerNgFactory);
        var elem = ref.location;
        _refs[elem] = ref;
        return elem;
      },
      updateElement: (child, idx) {
        var item = templateDisplays[idx];
        var ref = _refs[child];
        if (_subscriptions.containsKey(child)) {
          _subscriptions[child].cancel();
        }
        var compositeSubscription = _subscriptions.putIfAbsent(
            child, () => new CompositeSubscription());
        compositeSubscription.add(ref.instance.onClick.listen((_) {
          handleTemplateClicked(ref.instance.templateDisplay);
        }));
        compositeSubscription.add(ref.instance.onTemplateAccessoryAction
            .listen(handleTemplateAccessoryAction));
        ref.instance.templateDisplay = item;
        //ref.changeDetectorRef.detectChanges();
      },
      recycleElement: (child, idx) {
        var ref = _refs[child];
        ref.instance.templateDisplay = null;
        _pool.add(child);
      },
    )..scrollTarget = this.scrollTarget;
    _scroller.totalItems = templateDisplays.length;
  }

In this example, we're creating a TemplateContainer by using loadDetached. In the updateElement, we set the @Input ref.instance.templateDisplay = item;

This all works as long as we call ref.changeDetectorRef.detectChanges() but since that API is now deprecated I don't know how to force change detection.

I've tried changing the TemplateContainer to use OnPush as the docs suggest with no luck.

You can find an example of this component here: https://github.com/johnpryan/virtual_scroller_dart_example

The example uses an earlier version of Angular but exhibits the same behavior. Our real project is using Angular 5.3.0.

leonsenft commented 5 years ago

In general, the solution would be to inject ChangeDetectorRef and call markForCheck() to let Angular know that the component's view should be change detected (otherwise when a component is OnPush, it will only update after an input binding changes or an output handler is called).

For an imperatively created view to be reachable during change detection, it must be attached to a ViewContainerRef. This is not the case in your example, since you're attaching the root element of the view (ref.location) to the DOM manually. This means there's no connection between the views for change detection to propagate.

I'll review this case with my team and get back to you with a response. We may have overlooked this case when discussing the need for detectChanges(). This seems like a pretty rare and advanced use case. The problem with keeping detectChanges() is that it can easily be used to invalidate some of the guarantees of Angular's change detection cycle, which can lead to a variety of issues.

moczix commented 4 years ago

i got similar issue, i want to have somethig like webcomponents with angularDart, so i check if html has my chose selector, if so i mount it with loader.loadDetached and my CD didnt work. So all i had to do was this._appRef.registerChangeDetector(component.changeDetectorRef); and below is full code

_loadComponent<T>(ComponentFactory<T> componentFactory) {
  final existing = querySelector(componentFactory.selector);
  if (existing != null) {
    final component =
        _loader.loadDetached(componentFactory, injector: _injector);
    existing.replaceWith(component.location);
    this._appRef.registerChangeDetector(component.changeDetectorRef);
  }
}

it work even with onPush, of course you should add unregister cd when element is destroyed

leonsenft commented 4 years ago

Sorry @matthewtsmith, I forgot to reply here. @moczix's solution will work, but be advised this registers the component change detector at the root of your application, meaning all of the registered components will be change detected every pass (this differs from your implementation which only calls detectChanges() from updateElement()). This might not perform as well as your existing solution. To retain some of this performance, you could make your components OnPush, and call markForCheck() in place of detectChanges(). You can read more about OnPush here: https://github.com/dart-lang/angular/blob/master/doc/advanced/on-push.md

One idea we've been toying with is to add an API like registerChangeDetector to ViewContainerRef. This would give you the exact same capabilities, but the change detectors would only be run when the enclosing ViewContainerRef is checked. For Default change detection, this doesn't make much difference, but for OnPush it means you can skip all of these checks when the parent is skipped. This matters a lot for components with many child components that could be registered in this manner, such as a table. You don't want every cell in a large table being change detected every cycle.