formly-js / angular-formly

JavaScript powered forms for AngularJS
http://docs.angular-formly.com
MIT License
2.22k stars 405 forks source link

hideExpression exception when model null. #698

Open Omsad opened 8 years ago

Omsad commented 8 years ago

An exception is thrown when the model the hide expression uses is null. Please see here.

To reproduce click on the "Set Model to Null" button and then switch to your browsers console to see the error.

This looks to be caused by "const val = model[field.key];" line in the runFieldExpressionProperties method of the FormlyFormController controller.

I have updated the code locally to "const val = model == null ? undefined : model[field.key];" and this fixes the problem by treating a null model as an undefined property, I'm unsure however if this is the correct way to fix it.

Omsad commented 8 years ago

I ‘ve highlighted the problem line of code below from the FormlyFormController:

function runFieldExpressionProperties(field, index) { const model = field.model || $scope.model const promise = field.runExpressions && field.runExpressions() if (field.hideExpression) { // can't use hide with expressionProperties reliably const val = model[field.key] <- This is the problem. field.hide = evalCloseToFormlyExpression(field.hideExpression, val, field, index, {model}) } if (field.extras && field.extras.validateOnModelChange && field.formControl) { if (angular.isArray(field.formControl)) { angular.forEach(field.formControl, function(formControl) { validateFormControl(formControl, promise) }) } else { validateFormControl(field.formControl, promise) } } }

As you can see the issue isn’t with a property of the model being null but the model itself being null. The code makes the assumption that the model is always going to have a value which isn’t true in my case. Changing the line to “const val = model == null ? undefined : model[field.key]” keeps the behaviour consistent as far as I can see and allows for a null model.

Omsad

From: franzeal [mailto:notifications@github.com] Sent: 22 July 2016 18:09 To: formly-js/angular-formly Cc: Omsad; Author Subject: Re: [formly-js/angular-formly] hideExpression exception when model null. (#698)

Is there some reason you don't just use the function form http://docs.angular-formly.com/docs/formly-expressions#expressionproperties-validators--messages and safely resolve your model property from there e.g.:

hideExpression: function($viewValue, $modelValue, scope) { return scope.model && scope.model.baz === 'foobar'; }

kwypchlo commented 8 years ago

Why would you want to set your model to null in any case? Model needs to be an object, it can be empty object like {} or Object.create(null) but never null because if you decide to put some property on it, you will not be able to because you cannot store any property on null. If you want to clear the model completely, use an empty object as replacement {}.

Omsad commented 8 years ago

I use null as a representation of an unassigned object, e.g. it should never have properties set on it. By the time any properties on it are assigned, the model will have been changed to a real object so there will be no issue.

If at any point a property of the model is assigned and the model is null I would want an error to occur as that would mean there is a bug in my logic controlling that particular model.

If I was to use an empty object everything which was bound to it would have to check to see if the object was "empty" or "real", as different logic occurs depending upon the current value.

From a readability and code maintenance point of view having the extra logic to calculate the above is a pain because I would have to have it in multiple locations as I use multiple templates either bound directly to the "model", bound to the parent of the model through a property, or bound to a child property of the model.

These templates would then need to know the context of what was being bound to them as for some of the templates an empty object is a perfectly valid model to bind. This would introduce a dependence into the controller using them which could be forgotten / misconfigured.

kwypchlo commented 8 years ago

Well I'm not a fan of allowing null as a model - this will only cause hard to catch errors on any taken action. Putting null as a model and observing if errors are thrown is not a good way of debugging your code :) That said, your patch seems simple enough so I don't see a reason not to include it. I think we could use a second opinion here, anyone?

kwypchlo commented 8 years ago

@formly-js/angular-formly-collaborators second opinion anyone please?

kentcdodds commented 8 years ago

I would not recommend this change be made.