ngModelChange fires twice when the value for nimble-text-field is updated

[pakdev]

🐛 Bug Report

Noticed that I was logging twice in my ngModelChange handler when typing new values into a nimble-text-field

💻 Repro or Code Sample

https://stackblitz.com/edit/angular-ivy-ayatnv?file=src/app/app.component.ts

<nimble-text-field [ngModel]="value" (ngModelChange)="onValueChange($event)"></nimble-text-field>

public value = '';

onValueChange(value: string) {
    console.log(value)
}

🤔 Expected Behavior

ngModelChange should call my handler once per character change

😯 Current Behavior

ngModelChange calls my handler twice per character change

Behavior as demonstrated by the stackblitz link

💁 Possible Solution

N/A

🔦 Context

Trying to update a service value with the text field's new value

🌍 Your Environment

OS: Windows, Stackblitz
Browser: Microsoft Edge, FireFox

[rajsite]

Tried it in the example app and reproduce the behavior. I can see that the first time it is triggered it is the DefaultValueAccessor running:

The second time it is triggered is the NimbleTextFieldControlValueAccessorDirective that runs:

I expect Nimble's to be running, but don't understand why Angular is also binding the DefaultValueAccessor... The selector for the DefaultValueAccessor should not match nimble-text-field: https://angular.io/api/forms/DefaultValueAccessor#selectors

[rajsite]

Overview

Went down the rabbit hole 🐇 and have a theory of what is happening.

The current Textfield CVA extends Angular's DefaultValueAccessor directly. If you follow the inheritance tree you land on this magical comment:

Base class for all built-in ControlValueAccessor classes (except DefaultValueAccessor, which is used in case no other CVAs can be found). We use this class to distinguish between default CVA, built-in CVAs and custom CVAs, so that Forms logic can recognize built-in CVAs and treat custom ones with higher priority (when both built-in and custom CVAs are present).

Note: this is an internal-only class and should not be extended or used directly in applications code.

So my theory is that the way we are trying to leverage the DefaultValueAccessor gets Angular confused and injecting it's own DefaultValueAccessor along with the TextField CVA at runtime. We have a Custom CVA that Angular recognizes as a Default CVA based on the stuff we are inheriting from. (Hand wavy guess / theory, I haven't traced the exact mechanism). Edit: I think this theory / behavior is essentially confirmed by angular/angular#9146

Solution Fork Angular form CVAs

I made a branch where I vendor in the DefaultValueAccessor and remove any funny business linking back to Angular's heirarchy and it seems to work (open dev tools and type characters in the underline text field, notice one console log per character).

This is essentially the first product facing issue related to #732 which cites that we are un-supportedly extending Angular's private API the way we are doing CVAs.

This is similar to the radio_control_value_accessor we need to fork the Angular Form Directives we are interested in if we want to leverage their implementations for our web components. 😢

Maybe a submodule patch workflow and a separate package would be useful...

Non-solution Re-use Angular CVAs as hostDirectives

Unfortunately the Angular 14 hostDirectives feature discussed in #732 won't work in this case because the DefaultValueAccessor directive is not standalone and the Angular team is hesitant about making the form directives standalone

Solution Compose the Angular CVAs in wrapper Directives

I created a prototype branch of doing this with the DefaultValueAccessor. I found a TypeScript pattern to make sure we capture the API surface of the public API / that we can catch any public API deviations from the wrapper to the original at compile. I'd want to try a more sophisticated CVA to see if the approach is workable.

Solution Compose the Angular CVAs in wrapper Directives with Proxy

Example branch using es6 Proxies to forward everything automatically to an instance of the DefaultValueAccessor and making it lie correctly for instanceof checks.

[catalin-drulea]

This is something that is currently impacting the Asset Management team. Found this bug before creating a newer one on the same issue.

[jattasNI]

@catalin-drulea were you able to find a workaround for your use case?

I re-added the triage tag to remind the team to evaluate options and priority since this is cropping up again.

[ipopa144]

I'm also encountering the same problem when trying to add a search bar in the Feeds application. I haven't found any workarounds.

[jattasNI]

@ipopa144 good news: we expect this will be resolved with the Angular 15 upgrade since we're changing the implementation of directives. See #1655 for more info.