Form patchValue method throws unnecessary error

[edusoccer1121]

🐞 bug report

Affected Package

The issue is caused by package @angular/....

The issue is caused by @angular/forms

Is this a regression?

I don't think so.

Description

When the patchValue method on a form which contains a form group and the value passed is equal to null or undefined, error is thrown.

🔬 Minimal Reproduction

https://stackblitz.com/edit/angular-issue-repro2-hxfefx

🔥 Exception or Error

ERROR TypeError: Cannot convert undefined or null to object
    at Function.keys ()
    at FormGroup.patchValue (model.ts:1472)
    at eval (model.ts:1474)
    at Array.forEach ()
    at FormGroup.patchValue (model.ts:1472)
    at AppComponent.ngOnInit (app.component.ts:31)
    at checkAndUpdateDirectiveInline (provider.ts:217)
    at checkAndUpdateNodeInline (view.ts:440)
    at checkAndUpdateNode (view.ts:400)
    at debugCheckAndUpdateNode (services.ts:433)

🌍 Your Environment

Angular Version:

{
  "name": "angular",
  "version": "0.0.0",
  "private": true,
  "dependencies": {
    "@angular/animations": "9.1.2",
    "@angular/common": "^9.1.2",
    "@angular/compiler": "^9.1.2",
    "@angular/core": "^9.1.2",
    "@angular/forms": "^9.1.2",
    "@angular/platform-browser": "^9.1.2",
    "@angular/platform-browser-dynamic": "^9.1.2",
    "@angular/router": "^9.1.2",
    "core-js": "^2.5.7",
    "rxjs": "^6.3.3",
    "tslib": "^1.10.0",
    "zone.js": "^0.8.26"
  },
  "scripts": {
    "ng": "ng",
    "start": "ng serve",
    "build": "ng build",
    "test": "ng test",
    "lint": "ng lint",
    "e2e": "ng e2e"
  },
  "devDependencies": {
    "@angular-devkit/build-angular": "~0.7.0",
    "@angular/cli": "~6.1.1",
    "@angular/compiler-cli": "^6.1.0",
    "@angular/language-service": "^6.1.0",
    "@types/jasmine": "~2.8.6",
    "@types/jasminewd2": "~2.0.3",
    "@types/node": "~8.9.4",
    "codelyzer": "~4.2.1",
    "jasmine-core": "~2.99.1",
    "jasmine-spec-reporter": "~4.2.1",
    "karma": "~1.7.1",
    "karma-chrome-launcher": "~2.2.0",
    "karma-coverage-istanbul-reporter": "~2.0.0",
    "karma-jasmine": "~1.1.1",
    "karma-jasmine-html-reporter": "^0.2.2",
    "protractor": "~5.3.0",
    "ts-node": "~5.0.1",
    "tslint": "~5.9.1",
    "typescript": "~2.7.2"
  }
}

Anything else relevant?

[AndrewKushnir]

Hi @edusoccer1121, thanks for providing a repro. According to the type definition of the patchValue method here:

angular/packages/forms/src/model.ts

Line 1470 in a62c1c4

patchValue(value: {[key: string]: any}, options: {onlySelf?: boolean, emitEvent?: boolean} = {}):

the null and undefined values (that you try to use as an argument) are not supported and should actually trigger a TypeScript error. I've updated your repro (new repro is available here) to set an explicit type for the errorFormValue field and now you can see that the TypeScript error is produced. It looks like Framework works as expected and no additional handling inside the patchValue function is required (as these values are not supported by the function type definition), so I will close this ticket. Thank you.

[eduardosivd]

@AndrewKushnir you welcome.

Does this means I cannot received a property with null value and use the method, since it will fail and not update the form? In a API response with a null property the method would not update correctly the form. Meaning I would have to check each prop before using the patchValue method ?

Guidance on how to proceed when I can't control or check for nullness of every property on the value passed to the function.

[AndrewKushnir]

Hi @eduardosivd, thanks for additional information, I see your point now (I was confused originally due to the fact that a sub-group was passed into the patchValue call in your example). Due to the fact that the patchValue function is called recursively in the FormGroup and FormArray classes, the null and undefined may be present in the nested structure, so it would be ok from the patchValue function argument types perspective, but will fail at runtime.

I will re-open this ticket, so we can discuss it further.

Thank you.

[bbarry]

angular/packages/forms/src/model.ts

Lines 1558 to 1566 in 062b8d9

	patchValue(value: {[key: string]: any}, options: {onlySelf?: boolean, emitEvent?: boolean} = {}):
	void {
	Object.keys(value).forEach(name => {
	if (this.controls[name]) {
	this.controls[name].patchValue(value[name], {onlySelf: true, emitEvent: options.emitEvent});
	}
	});
	this.updateValueAndValidity(options);
	}

would a fix be something like:

 patchValue(value: {[key: string]: any}, options: {onlySelf?: boolean, emitEvent?: boolean} = {}): 
     void { 
   if (value === null || value === undefined) { this.reset(options); } // or return? semantics not clear
   Object.keys(value).forEach(name => { 
     if (this.controls[name]) { 
       this.controls[name].patchValue(value[name], {onlySelf: true, emitEvent: options.emitEvent}); 
     } 
   }); 
   this.updateValueAndValidity(options); 
 } 

Or would it be better to introduce a stricter type:

interface RecursiveRecord {
  [K: string]: string | boolean | number | RecursiveRecord | string[] | boolean[] | number[] | RecursiveRecord[]
}

though this could break code which currently does function using null or undefined on the innermost FormControl objects... (and does not work for nested form arrays as is; it would require a significantly more complex type)

[jpett]

Hi all,

we have the exact same problem in our solution. Our api may return null-values for child FormGroups which currently results in this error when calling patchValue. Since we use FormGroups that are dynamically composed at runtime it makes implementing workarounds for this issue even a little more tedious :)

@bbarry: Thanks for your suggestion. It's indeed an interesting question how to handle this case. In my opinion you could either call this.reset(options) as you have suggested or simply do nothing:

patchValue(value: {[key: string]: any}, options: {onlySelf?: boolean, emitEvent?: boolean} = {}): 
     void { 
   if (value !== null && value !== undefined) {
     Object.keys(value).forEach(name => { 
       if (this.controls[name]) { 
         this.controls[name].patchValue(value[name], {onlySelf: true, emitEvent: options.emitEvent}); 
       } 
     }); 
     this.updateValueAndValidity(options); 
   }
 } 

In my opinion, doing nothing would still align well with the method's purpose as described in the docs:

Patches the value of the FormGroup. It accepts an object with control names as keys, and does its best to match the values to the correct controls in the group

Doing nothing would in this context mean we update all the fields and data that are available and leave all other fields unchanged. I guess this would also have less side effects in respect of existing applications.

@AndrewKushnir In my opinion the priority is a little higher ;) Actually, you can't use formGroup.patchValue as soon as you have an optional child object in your data that is null or undefined. However, this is a rather usual case and forces us to use workarounds like updating individual sub-FormGroups or replacing nulls with values like {} or similar. I would be happy to create a PR as soon as a solution was agreed upon.

[AndrewKushnir]

Actually, you can't use formGroup.patchValue as soon as you have an optional child object in your data that is null or undefined. However, this is a rather usual case and forces us to use workarounds like updating individual sub-FormGroups or replacing nulls with values like {} or similar. I would be happy to create a PR as soon as a solution was agreed upon.

Thanks for additional information @jpett.

I think the null/undefined cased is tricky, because there might be different expectations (both potentially valid ones):

• the value should be ignored (thus the entire control sub-tree should remain as is)
• controls' subtree should be reset/removed

Given that the patchValue API is pretty generic, I don't think we can make that decision on the patchValue level and it should be up to developers on how it should be interpreted (reset/remove/ignore). Having extra flags/options on the patchValue method doesn't seem like a good option too as it would complicate the API (we'd need to revisit other APIs to keep them consistent) and we may still face more use-cases in the future where undefined values require special handling (so we'll need to complicate the API further).

Currently I'm leaning towards having a more descriptive error message (vs just crashing on trying to iterate over undefined values) thrown from the patchValue methods when we detect that provided value can not be handled (i.e. it's not iterable) and updating patchValue docs to include a note on undefined/null fields.

[jpett]

Thanks @AndrewKushnir for your elaborate response.

I definitely see your point. However, I think the current implementation has the inherent problem that you cannot manage objects with nullable object properties which is a pretty common case when modelling business objects. Just consider something like a customer who may optionally provide a billingAddress:

interface Customer {
  id: number;
  firstname: string;
  lastname: string;
  shippingAddress: Address;
  billingAddress: Adress | null;
}

Currently, you cannot create FormGroups for this interface and patch customer data with a billingAddress of null without getting a runtime error from setValue or patchValue, even though the object is consistent and valid. You have to either patch every FormControl and the FormGroups individually or do something like this before patching the data:

if (customer.billingAddress === null) {
  customer.billingAddress = {};
}

Both solutions feel not really elegant. I think if we would change patchValue in order to do nothing instead of throwing an error we could fulfill all expectations:

• If the value should be ignored you can just use the revised version of patchValue
• If the control's subtree should be reset, you can call formGroup.reset() and then formGroup.patchValue()

I am aware (and thankful!) that your team has to think a lot about breaking/changing behaviour of existing code. The only change of behaviour I can think of in this case is that currently patchValue might be interrupted by the error that is being thrown, which might result in a FormGroup that is only partly patched. It depends on the order of the FormGroup's controls, which controls were patched and which were not (I just checked with a little sample):

Sample FormGroup 1:

this.formGroup = this.formBuilder.group({
  firstname: null,
  lastname: null,
  shippingAddress: this.formBuilder.group({...}),
  billingAddress: this.formBuilder.group({...})
});

If we patch customerData with a billingAddress of null into this FormGroup, and if the FormGroup was empty or reset before, the FormGroup would actually still be filled correctly because the error is thrown when trying to patch the last control of the FormGroup

Sample FormGroup 2:

this.formGroup = this.formBuilder.group({
  billingAddress: this.formBuilder.group({...}),
  firstname: null,
  lastname: null,
  shippingAddress: this.formBuilder.group({...})
});

Patching the customerData into this FormGroup would result in an empty form because the error is thrown when trying to patch the first control.

Obviously, there are also cases were the relevant FormGroup is somewhere in between which would result in a form that is only partly filled. So Sample FormGroup 1 resembles a special case that 'accidentally' results in a correctly filled form.

I don't see that anybody would knowingly make use of patchValue's current behaviour and rely on patchValue throwing an error. You woul have to write code like this:

try {
  this.formGroup.patchValue();
}
catch {
  // Handle error explicitly
}

I would definitely expect people writing code like this when using setValue since this method explicitly states it expects a matching object.

Sorry for getting into so much detail about this but writing everything down just helped me realising some of the implications :)

TLDR

In summary, I see your point, but as described above I also think that a FormGroup should be able to handle objects with nullable object properties. In the end, it may indeed be the best solution to introduce a new option flag to the existing options object of patchValue, e.g. skipNulls or similar. You mentioned you were afraid of making the API more complicated or inconsistent. Maybe you could elaborate a little on that if you find the time.

One last thing: Looking at the angular roadmap I saw this very exciting point:

Better developer ergonomics with strict typing for @angular/forms

That would definitely be a great feature and I think it would be even better if it would also support nullable object properties. ;)

Thanks again for your great work!

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