Hi @N2D4, thanks for creating this PR 👍

I noticed that it's in Draft mode (i.e. it's probably not fully ready for review), but I wanted to share an idea. If I understand correctly the first "writeValue" with the undefined value comes as a result of FormControl instance creation in ng_model.ts here (with no value). I was thinking if we can instantiate FormControl there with some "special" initial value and check for it before calling the writeValue method in the setUpControl function, i.e. something like this:

// in packages/forms/src/directives/ng_model.ts:

export const INITIAL_CONTROL_VALUE = {};

...

public readonly control: FormControl = new FormControl(INITIAL_CONTROL_VALUE);


// in packages/forms/src/directives/shared.ts:

if (control.value !== INITIAL_CONTROL_VALUE) {
  dir.valueAccessor.writeValue(control.value);
}

Would that work? (If that works as needed, it should avoid passing configuration params though a few function calls)

Thank you.

Hi @N2D4, thanks for creating this PR 👍

[...]

Would that work? (If that works as needed, it should avoid passing configuration params though a few function calls)

Thank you.

Sure, implemented. I also added an integration test (/modified an existing one), so this is now ready for review 👍

Hi @N2D4,

I performed initial review and added the "breaking changes" flag to this PR, since the new behavior is different from the old one (i.e. there us no additional writeValue call) and developers may rely on that, thus we need to highlight this change in the Breaking Changes section of Release Notes.

I've also ran a set of tests in Google's codebase (internal-only link) and it looks like the problem is a bit more tricky. I found some additional cases were this change would be breaking. Here is a couple snippets that illustrate the problem:

Component's template:

<input
  myValidator
  [(ngModel)]="someField"
/>

Validator (simplified version):

@Directive({
  selector: '[myValidator]',
  providers: [{provide: NG_VALIDATORS, useExisting: MyValidator, multi: true}]
})
export class MyValidator implements Validator {
  validate(control: AbstractControl) {
    console.log(control.value);
    return null;
  }
}

The problem is that the control.value value in the validate function would be INITIAL_CONTROL_VALUE, i.e. just an empty object, thus causing some code to fail (the code that may not expect to see the object there at all). Also from API perspective having the {} here is quite unexpected. Also the question is whether a validator (sync/async) should actually be invoked in that case as well (I believe it should not)?

Given the fact that the INITIAL_CONTROL_VALUE may leak in some other places (possibly in ngOnChanges hook if you use ngModel as an input on a custom element, like <my-comp [(ngModel)]="someField">), I'm wondering if we should go back to the flags-based approach.

In any case, I think it'd be great to add the above mentioned example as a test case (it should be useful to count the number of times sync/async validator was called) and evaluate whether INITIAL_CONTROL_VALUE-based approach may work or other solution should be explored.

Please let me know your opinion.

Thank you.

Hi @AndrewKushnir,

Sorry for the delay, since this is not going to happen before v11 anyways I ended up re-prioritizing things but I'm back to work on this now.

As an open-source intern I don't have access to the internal link, but I could reproduce the error you mentioned. I agree that the validator shouldn't be called there, and added a unit test to make sure it's not. Yes, I believe in the ngOnChanges example you mentioned the value would leak as well, so I reverted back to the flag approach. While it may or may not be possible to work around this while keeping the sentinel approach, the code would probably become much more complicated than it already is and any future change might leak the value again. Thanks for the feedback.

Now that we also skip validators when the form control is created, I took a closer look and decided to rename the flag to skipValueInitialization as that fits it better semantically. It should be passed to addControl (and the other methods) if and only if the value will be set/updated/initialized immediately after the call (as compared to already having been initialized). I've been thinking that we could also pass an initialization value instead - if given, addControl itself would initialize the control with that value first. It would complicate some of the logic in setupControl and addControl by a bit, but removes that weird invariant of "if you pass this flag, you MUST update the value immediately". Let me know if you find one to be better than the other.

There was one test that relied on the old behavior of calling validators synchronously during ngOnChanges when the component is first created. The call with argument null was already asynchronous for non-standalone, but synchronous for standalone components. Since the null call no longer happens, no such validator call will happen anymore before ngOnChanges returns. I don't think this should cause a lot of breakage in consumer unit tests, but I still thought it's worth pointing out.

Then, any news about merging?

Hi @N2D4 and @AndrewKushnir 😄

I saw this PR as a result of debugging this issue: #43718

The problem I had was due to the fact that writeValue is called twice: once with null, once with an empty string, so the empty string overwrites the default value.

If I read this PR correctly, it should avoid the first writeValue call, which is the one setting null. My question is: shouldn't the second call (the real one which won't be skipped) set the value to null instead of an empty string?

There could be multiple reasons for that!

First, that's how Reactive Forms work, but right now the default value for a FormControl is null while for ngModel it's an empty string. Isn't that an inconsistency?

Second, imho it makes more sense for some controls to be null rather than an empty string (eg. checkbox, which is a boolean, or a custom control which could accept any value). Since I know that there's a lot of work going on involving typed forms, I think having better types in Template-driven forms (at runtime) should be an added benefit.

I don't know if this directly affects this PR, but since it would eventually fix an issue from years ago, you never know!

What do you think about it? :)

This fix requires some additional design work / research as the current approach is too breaking to land as-is.

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.}