canjs / can-define

Observable properties
https://canjs.com/doc/can-define.html
MIT License
15 stars 7 forks source link

Should define support nullable and required properties? #173

Open justinbmeyer opened 7 years ago

justinbmeyer commented 7 years ago

@bmomberger-bitovi commented on Sun Jun 12 2016

[This was originally incorrectly filed as can-define issue #20]

Currently in can-map-define, setting a Type attribute on a property ensures that after setting that property using .attr(), the property's value will be an instance of that Type class, even if the value passed to .attr() is null or undefined. The property's value starts as undefined if neither Value nor value is set, but once set, the only way to get a "null value" (null or undefined) for that property is to use .removeAttr().

Likewise, setting Value or value enforces an initial value, but does not resist nor reset to default if removeAttr() is called for the property; the result is a value of undefined.

I think that it would be helpful to have slightly better type/value enforcement options. Since sometimes we want to have a Maybe<Map> (as static-type functional programmers call it) where a property is assumed to have either an object of the parameter type or a null value, it may be sensible to allow for something like nullable: true as a define attribute. This would prevent implicitly calling the constructor if the value is a null value.

Conversely, if users wanted to guarantee that a property always had a particular value, it would make sense to have a required: true attribute, which either disables .removeAttr() or follows it up with a call to the appropriate Value or value constructor.

Does this seem like a good idea?


@justinbmeyer commented on Fri Sep 02 2016

I think some of this issue will be closed by: https://github.com/canjs/canjs/issues/2316. Anyway you can update the issue to reflect things you'd like to see outside of that issue?

justinbmeyer commented 7 years ago

Warnings if required: true

VM = DefineMap.extend({
    first: {
        required: true
    }
})
validate(VM);

Component.extend({
    tag: "my-name"
    ViewModel: VM
})

<my-name {first}/>
andrejewski commented 7 years ago

@Macrofig, any suggestions in relation to can-validate?


I think there are really three issues here:

  1. Adding a nullable property
  2. Adding a required property
  3. Having a property reset to the initial [v|V]alue on removeAttr()

Right now I think required can be added much more easily than nullable, as nullable is kind of the way things are now and can't be enforced as rigidly. Regarding property removal, I don't know if a reset would be expected behavior.

I'd like more community feedback about this before we move forward implementing.

Macrofig commented 7 years ago

Just to be clear, when I use required I am referring to the proposal of adding required to can-define. I'll refer to can-validate's required as presence.

Using can-define-validate-validatejs on a DefineMap adds errors and testSet. errors is a getter and testSet is called as needed. I think that means that, if a property is required, can-validate won't ever throw an error if presence is set. That might be fine as the behavior should be pretty obvious to a our end-users.

That said, I think most of the use cases for required result in a behavior of setting an error. So, I agree with @andrejewski's list, I think required is a separate issue. The removeAttr reseting to default value is a different behavior that I would not expect if I set required: true. Maybe, instead of required, we just need a better named key... something that conveys the original intent in https://github.com/canjs/can-define/issues/20 ... like default-to-value-when-null. ¯_(ツ)_/¯

tl;dr

I think the idea of a required property is definitely needed (we've all come across a time where replacing null/undefined with a default value would have been useful). I think the key name of required may confuse some folks because we associate that key with validation behaviors.

rjgotten commented 6 years ago

Regarding property removal, I don't know if a reset would be expected behavior.

For what it's worth: I'd expect removeAttr to actually remove the attribute entirely, but if it is later recreated via an attr("prop", undefined) call, then the define.prop.[V|v]alue should spring back into use.

If you want to reset properties to their defaults; implement an actual reset.