ChangeDetectionStrategy.OnPush not running properly with @Input modified programmatically (outside of a template binding)

[UserGalileo]

I'm submitting a...

[X] Bug report  

Current behavior

I'm trying to set an Input property of a component which is projected into another one. I tried, for performance reasons, to use ChangeDetectionStrategy.OnPush on the child component, but I've come across a strange bug.
With ChangeDetectionStrategy.Default everything works fine, but using OnPush I get a strange behavior. It appears that the OnPush component doesn't receive the new input property. Please see my example which clarifies the problem.
I think the problem could be confined in ContentChildren, ViewChildren (same problem) or QueryList, and I believe it's the latter but I'm not sure about it.
It's been clarified that OnPush just looks at inputs which are set in the template and not via JavaScript. Although this kinda makes sense thinking about the current framework's logic, it doesn't in a sense that the child component using OnPush isn't "thinking with its own brain", but it's the parent who decides if re-rendering should occur for it, based only on its template bindings.

Expected behavior

The component's Input property should be received and the component re-rendered even with ChangeDetectionStrategy.OnPush.

Minimal reproduction of the problem with instructions

This is the demo I made, with a simple component which is responsible for the state of all its components which are being projected inside of it.

https://stackblitz.com/edit/angular-accordion

Please see the comment in toggle.component.ts and try changing Default to onPush

What is the motivation / use case for changing the behavior?

Compound components often need to be used with OnPush in order to optimize performance with large applications.

Environment

Angular version: 5.0.0


Browser:
- [X] Chrome (desktop) version 62

[alexzuza]

It appears that the OnPush component doesn't receive the new input property

Indeed there are no input properties active here. Angular doesn't produce update logic for @Input in ngfactory if it doesn't appear in template. For example, compare it with

<toggle title="Title 1" #t1 [active]="t1.active" ></toggle>

https://stackblitz.com/edit/angular-hgndju?file=app/app.component.html

[UserGalileo]

Actually that "active" property does appear in the template, there's an ngIf based on it on the component itself, which should be everything I need in order to have the component re-rendered :) I really can't see why I should use your technique, passing a property via ContentChildren is really what makes Compound Components a great design pattern and that should be the perfect usecase for OnPush. The component itself has no problem with OnPush, it DOES have problems with ContentChildren/ViewChildren or QueryList, I don't know why.

OnPush should regard only the specific component's business, it shouldn't impact on how the property is passed. I can't see why having the active property in the parent's template should care for the child component, what matters is that its input property has changed, no matter how. Passing properties without the template binding is needed in a lot of situations, the end user shouldn't be responsible for setting up a workaround :)

[mlc-mlapis]

@UserGalileo ... but you ignore how OnPush principally works ... it follows changes on @Input()s ... especially if a reference has changed on @Input() when you pass object or array ... and if there is no change on them then CD is not run. This is all.

[UserGalileo]

@mlc-mlapis I don't ignore how OnPush works, I use it really often. I'm sorry, but did you see my example? The input property IS changing, but instead of using template binding, since I get the components with ContentChildren I change the property imperatively:
toggle.active = false
The input is passed down again to the component but it doesn't re-run CD. Also there are strange behaviors when clicking on an item, clicking on another one and re-clicking on the previous one.

See accordion.component.ts which is where the input property is passed down at each click.

[sarunint]

@UserGalileo I'm not entirely accurate on this topic either, but I'll say that you are modifying the class member directly which Angular's change detection mechanism cannot detect that. It can detect only change that occur to the binding.

Again, not entirely sure. I should summon @MaximusK, the change detection god, for this topic.

[UserGalileo]

Thank you @sarunint I appreciate that :) However, if it's intended as a feature, I must say that this should be changed, modifying instance members shouldn't be a problem at all and shouldn't interfere with CDS.OnPush!
Waiting for the change detection god to clarify, thanks ;)

[mlc-mlapis]

@UserGalileo ... it is also not related to ContentChildren or ViewChildren at all ... if you add the following ngOnInit to ToggleComponent you can see that CD is also not invoked when active is asynchronously switched to true = equivalent of a click ...

export class ToggleComponent {
  @Input() active = false;
  @Input() title = "Default title";
  @Output() setActive = new EventEmitter();

  ngOnInit() {
    setTimeout(() => {
      console.log('Async call ...');
      this.active = true;
    }, 1000);
  }
}

[alexzuza]

As i wrote early angular won't check your active input because there is nothing to check.

So it means that angular won't call updateProp for your inputs and therefore your component will be hidden change detection.

We shouldn't change component property directly from any place if we want to abide by the unidirectional data flow

[alexzuza]

One more solution for you

toggle.component.ts

_active: boolean = false;

@Input()
set active(val) {
  if (this._active !== val) {
     this.cdRef.markForCheck();
  }
  this._active = val;
}

get active() {
  return this._active;
}

constructor(private cdRef: ChangeDetectorRef) {}

https://stackblitz.com/edit/angular-m9gamy?file=app/toggle.component.ts

[UserGalileo]

@alexzuza I get how OnPush works, that's why I marked it as a bug, I don't feel it's right :) Even if I don't pass the prop from the template, it should be change detected. I get that Angular is template-focused, but this is kinda limiting.
Also, this isn't breaking unidirectional data flow at all: the child emits an event which is subscribed to the parent, and the parent is passing down props again. This is an everyday-situation with other frameworks like React, and ContentChildren is the only way for handling the scenario and having the same result in clarity :)
Thanks for the alternative solution with setter/getters, but I'm not actually developing anything with that component at the moment. I'm not trying to fix THAT component, I'm trying to say what I feel about this situation and what is wrong with Change Detection (obviously imho).
Since we don't have anything like shouldComponentUpdate in React, OnPush is the best thing we can do in order to have performant components: I'd like to work with it without any workaround ;) And if it's intended to work like this (like others said), maybe we can discuss that and make CD a little better!

[maxkoretskyi]

@UserGalileo

@alexzuza is right, the compiler doesn't have a way to generate necessary information for checking the bindings since it can't find these bindings in the template.

Even if I don't pass the prop from the template, it should be change detected

OnPush is tightly bound to the input bindings. What's important is that Angular checks the second part of the binding (prop in the example below), not the first (i):

<child [i]="prop">

to determine whether the change detection should be run for the child component. And it does so when checking parent component. If you don't show the compiler what parent property should be used to update child input binding, it can't generate necessary information used when checking the parent. So inspecting @Input on child components isn't enough. That's the mechanism of change detection and I don't see any way it could be changed. You can read more about change detection operations in Everything you need to know about change detection in Angular.

Also, here is the simple example that demonstrates the limitation. The AComponent won't render updated name in 2 seconds because of OnPush. If you remove OnPush, the change will be propagated. That's the limitation of working with dynamic components as well.

@Component({
    selector: 'my-app',
    template: `<a-comp></a-comp>`
})
export class AppComponent {
    @ViewChild(AComponent) a;
    constructor() { setTimeout(() => { this.a.name = 'updated' }, 2000); }
}

@Component({
    selector: 'a-comp',
    template: `<h2>I am A component, name is: {{name}}</h2>`,
    changeDetection: ChangeDetectionStrategy.OnPush
})
export class AComponent {
    name = 'Jane';
}

You can read more about updateDirectives function shown on @alexzuza's screenshot in The mechanics of property bindings update in Angular. For the most comprehensive overview of change detection read These 5 articles will make you an Angular Change Detection expert.

[UserGalileo]

Hi @MaximusK and thanks for your time, I also read your blog posts time ago and I found them very useful and well written. However, I just don't feel this as a reliable behavior at all...

Inspecting @input on child components isn't enough

That's exactly what happens in React! In React there are 2 features we can use to handle this scenario: PureComponents, which shallow check the properties, and the shouldComponentUpdate lifecycle hook for "hacking" into the rendering process. In Angular, since we have nothing that does the check for us inside the component and since we haven't got a hook for the rendering process, PureComponents are meant to be those ones with CDS.OnPush, why wouldn't it be the right thing to inspect inputs on child components?

Please, don't get me wrong. I love both Angular and React, sorry for bringing other libraries to the table, but I feel this is kinda limiting. See the accordion example: the logic behind it should be completely hidden by the accordion itself, it shouldn't be exposed to the developer. The dev shouldn't care about that input property, it should be passed down from the accordion itself. And as I can see, the only ways to make it work flawlessly are:

1. Stop using OnPush on components which may be projected (but who knows? At that point, I'd have to stop using OnPush everywhere since everything can be projected)
2. Do some ""hacking"" with the change detector, which doesn't feel right to me and shouldn't be abused (and in large applications, I just can't see how to not abuse of it)

[UserGalileo]

The third and most distant option I see is to have a third CDS which just removes CD and listens to the component's inputs and re-renders, nothing more. But I'm thinking out loud :)

[maxkoretskyi]

@UserGalileo

and the shouldComponentUpdate lifecycle hook for "hacking" into the rendering process. In Angular, since we have nothing that does the check for us inside the component

There's the ngDoCheck on a component similar to shouldComponentUpdate in React which is called even if change detection is not run. You can utilize it in your case:

export class ToggleComponent {
  _active;
  @Input() active = false;
  @Input() title = "Default title";
  @Output() setActive = new EventEmitter();

  constructor(private cd: ChangeDetectorRef) {}

  ngOnInit() { this._active = this.active;  }

  ngDoCheck() {
    if (this._active !== this.active) {
        this._active = this.active;
        this.cd.markForCheck();
    }
  }

Read If you think ngDoCheck means your component is being checked — read this article for more information on this hook.

See the accordion example: the logic behind it should be completely hidden by the accordion itself, it shouldn't be exposed to the developer

Did you check how ngx-bootstrap implements accordion with templates?

why wouldn't it be the right thing to inspect inputs on child components?

Because as I explained in my answer, the decision to run change detection or not on a child component is made when checking parent (component's properties). Since unidirectional data flow defines strict update from top to bottom, this checking strategy makes perfect sense. Although it may be limiting at times as you noticed.

[UserGalileo]

@MaximusK
My accordion was just an example made for the purpose, it's not that there aren't other ways to do it! Compound components are a beautiful design pattern to me, which makes a lot of components reusable, not only the accordion :) At this point I'm asking myself if OnPush really makes sense at all, I just wouldn't rely on this behavior honestly... Properties shouldn't need to be bound to the parent's template in order to re-render the child, for me :) I absolutely understood that that's how CD works for now but maybe something else could be done in order to have real pure angular components. I'm absolutely no expert on CD's internals, so I'm just throwing a tought: wouldn't it make sense if components marked with OnPush (or with a third strategy, let's say, CDS.Pure) broke their bond with the parent's change detector and put listeners on their input properties? This way, unidirectional data flow would remain, access to the parent component via DI could be disallowed, and this components would have a separate change detecor which wouldn't start checking from the parent, but from the component's inputs. Would it make sense? Just thinking out loud again :)