[ivy rc4] new unbound (static) input handling breaks existing code and makes component lifecycle inconsistent

[Phil147]

🐞 bug report

Affected Package

Is this a regression?

No.

Description

In the ivy compatibility docs you can find this important sentence about a change coming with v9/ivy:

Unbound inputs for directives (e.g. name in <my-comp name="">) are now set upon creation of the view, before change detection runs (previously, all inputs were set during change detection).

We ran into a lot of issues with this, e.g. when you use a ViewChild query in an input setter.
I created a really simple example showing the problem and that the same code works for v8 but not in v9 (couldn't reproduce it in stackblitz)
https://github.com/Phil147/unbound-input-problem-v8
https://github.com/Phil147/unbound-input-problem-v9

The example is very simplified and not best practice but imagine you want to call a function of another custom component you query here.
In the v9 repo you will get an error once you start the app with ng serve because the ViewChild query is not yet resolved and thus undefined.

With v8 this works fine because the inputs are set during first change detection and the ViewChild query was resolved (with static: true). With v9 this code breaks but only in the particular situation when the input is not used with a binding but with a static value. We only found this issue by accident because in our unit tests we typically used bindings, but one specific example in our documentation (of our component library) failed during runtime.
This is a very confusing behavior now that the timing changes depending on how the inputs are used in templates. For me it would make more sense to keep this consistent.

From what I found this change was made to get better performance because static inputs don't change and you can leave them out during change detection, which makes total sense.
I wonder if it might be possible to still use the information you gather about static attributes but run them together with all other inputs during the first change detection run and then ignore them instead of moving their calls before the change detection happens.

🔬 Minimal Reproduction

works fine:
https://github.com/Phil147/unbound-input-problem-v8
same code breaks in v9:
https://github.com/Phil147/unbound-input-problem-v9

🌍 Your Environment

Angular Version:

9.0.0-rc.4
Angular CLI: 9.0.0-rc.4
Node: 12.13.0
OS: darwin x64

Angular: 9.0.0-rc.4
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.4
@angular-devkit/build-angular     0.900.0-rc.4
@angular-devkit/build-optimizer   0.900.0-rc.4
@angular-devkit/build-webpack     0.900.0-rc.4
@angular-devkit/core              9.0.0-rc.4
@angular-devkit/schematics        9.0.0-rc.4
@ngtools/webpack                  9.0.0-rc.4
@schematics/angular               9.0.0-rc.4
@schematics/update                0.900.0-rc.4
rxjs                              6.5.3
typescript                        3.6.4
webpack                           4.41.2

[pkozlowski-opensource]

Another case pointing to the same root cause (inputs from static attrs set "too early") in #34992

[shlomiassaf]

Jumping in as well.

This is super weird and going to break a lot of things out there

It means that behaviour wise

<div myDirectiveInput="abcd"></div>

IS NOT EQUAL TO:

<div [myDirectiveInput]="'abcd'"></div>

A good example is the CDK's coercion utils, for example the boolean coercion.

It accepts myAttr="false" as well as [myAttr]="false", both are false.

Now, anywhere someone implemented a reaction to this change via a property descriptor (setter) it will probably break behaviour cause things he expects to be there are probably not.

It's breaking a lot of existing logic while behaving differently for the same input (lifecycle wise).

I think this must be changed in 9.0.2 and not 10, cause up till 10 it will just get worse with people adopting it.

[shlomiassaf]

Here's another, rather complex, scenario which i'm required to handle things in the setter.

I have a grid library which is a rather complex component.

The grid is designed to show template it get's from the outside (for rendering cells, rows, etc...).
These templates are shared, across modules and across change detector levels.

In other words, the grid might render templates from view's that branched out of the grid's view, i.e. their change detectors also branched out. It might get more complex and nest internally...

Now, when the grid want to invoke a change detection cycle it will not trigger the change detection in the template, unless explicitly invoking that on the embedded view created, so far not a big issue.

The problem is when the created template/component is changed, usually manual code (updating properties on the component ref instance).
The change is usually done from the grid but the component updated is using a different CD. The grid will automatically run a CD cycle after the update but since the component is not part of it, the property will have the new value but it will not reflect on the UI of that component and ngOnChanges will not reflect these changes.
This is why the component will need to run a CD cycle using it's own change detector ref.
Note that within the setter it's impossible to figure out if the input value comes from a static value or a dynamically bound value, further more, understanding if the first CD cycle ran or not is not straightforward and quite verbose to figure out (through a flag set in AfterViewInit)

Of course, each component can expose its change detector ref so any update from grid can also trigger the CD, but that's just not right as the CDRef of each component should be private.

In general, most simple scenarios does not notice the implications of this issue but when imperative component creation is used, especially one that cross change detection boundaries, this is a big deal.

[pkozlowski-opensource]

Confirming that this is a breaking change in ivy: the timing of static inputs assignment changed and now it is happening much earlier (as compared to VE). This creates all sorts of problems in practice, but the one that comes up quite often is the assumption of static queries presence when an input setter is invoked: https://stackblitz.com/edit/angular-czurvn?file=src/app/app.module.ts

I think that the worst here is lack of predictability - a given setter will "work" or not depending on how input is provided (static vs. dynamic). I think that we should be more predictable here and set static inputs on the first update pass.

Marking it as regression. It is not a 5-minutes fix but we should be able to restore compatibility with VE. For now there is a relatively easy work-around - move to the logic to the ngOnChanges.

[shlomiassaf]

@pkozlowski-opensource Any update on this?

It's a real deal for us, we have a large project and we use get/set descriptors a lot.

The main problem for us is the lack of predictability, sometimes it works some times it does not.

While moving to ngOnChanges looks like a quick fix, it is not (for us).
It might be for 1 or 2 components but on scale it becomes hard because you also move logic here, it's not a simple code transformation... state is involved.

The other quick fix we have is to ignore all static bindings to angular inputs. We will force binding and loose the extra perf of one-time binding of static values.

<my-cmp [myInput]="'myString'"></my-cmp>

Can this be in the next v10.x minor/patch?

I understand you might have greater priorities but you stated it should be an easy fix and frankly this un-deterministic behavior is (IMHO) a major issue for a framework.

Thanks!

[bernatgy]

I don't understand why this has severity1: confusing. As @shlomiassaf predicted our company adopted, and now systems are silently breaking. This should be severity3: broken.

Disabling Ivy seems to be our only non-hacky option here, and to be clear, this is now the third or fourth time where disabling the main feature of Angular 9 had to come up as a possible resolution.

[hugoj-goncalves]

Sad news that this is just a medium priority issue
Guess I'm stuck and won't update for quite some time
I don't have the manpower to convert, fix and test all the applications that assumed angular bindings would stay the same way

[shlomiassaf]

Frankly I think this issue is not prioritised due to lack of visibility.

The angular team has made a promise to keep its predictable and deterministic.

This is a real surprise to me.

I can only assume they don't code this way at google.

[webmandman]

I believe if inputs for directives are set during change detection I would not have to implement it this way:
@Input() address: Address | null = null; to satisfy the strict option "strictPropertyInitialization". I've even tried with bang ! operator and ? operator, but these two present another problem in strict mode. See my simsple example here: https://stackblitz.com/edit/angular-ivy-m5wsy2?file=src%2Fapp%2Fapp.module.ts

[why520crazy]

any news?

[pkozlowski-opensource]

Ivy changed the timing of inputs with static values, it is true. But we've been living with this changed behaviour for almost 4 years now and the last comment on this issue is ~2 years old. Based on this it is fair to assume that most of the ecosystem adjusted to this change.

More importantly - we don't want directive authors to make assumptions about the precise timing on the inputs settings. The only "safe" way of reading / responding to input changes is ngOnChanges. It should not be done in an input setter.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}