ampersand-state icon indicating copy to clipboard operation
ampersand-state copied to clipboard

Required Props & Empty Attrs

Open mysterycommand opened this issue 10 years ago • 4 comments

Hi. I'm trying to create a read-only model with required properties. I would expect that attempting to instantiate such a model without the required properties in it's initialization object would throw an error, but it doesn't. I have the following:

var AmpState = require('ampersand-state');

var ReadOnlyModel = AmpState.extend({
    idAttribute: 'requiredProp',
    props: {
        requiredProp: {
            type: 'string',
            required: true,
            allowNull: false,
            setOnce: true
        }
});

var oneModel = new ReadOnlyModel({});

Shouldn't that throw an error?

mysterycommand avatar Aug 27 '14 21:08 mysterycommand

I created a little test, initing a model that defines required props with an empty object:

https://github.com/mysterycommand/and-collection-bug

$ git clone [email protected]:mysterycommand/and-collection-bug.git
$ cd and-collection-bug
$ npm i
$ npm test

Note the first failing test: "PersonModel should throw when required props are not present".

mysterycommand avatar Aug 28 '14 05:08 mysterycommand

Hey @mysterycommand,

So this is kinda interesting, and I think you've found something we probably want to think about a little harder.

What's happening is this: required is only checked when you actually try and set a property. So in your example above:

  • new ReadOnlyModel({}) will not trigger an error
  • new ReadOnlyModel({ requiredProp: undefined }) will trigger an error, because a required field set is attempted to be set to set to undefined.

Also, the current behaviour of required: true is if the field hasn't been set to use the default value, in this case it's empty string (defined in the datatype).

It seems like we'll need to think a bit about making it clearer the purpose of required/default/etc, there's at least one more somewhat related discussion here: #48

latentflip avatar Aug 28 '14 08:08 latentflip

@latentflip, thanks for the feedback. I see what you're saying. I guess I am just expecting that if I create a required property, it'll be there on my model. How would new ReadOnlyModel({}).requiredProp; get resolved? Look up a default? What if I set the default to null, and allowNull to false?

mysterycommand avatar Aug 28 '14 16:08 mysterycommand

Yep, as related to #60: would be great if required means that you can't instantiate a state without a value.

dminkovsky avatar Sep 17 '14 21:09 dminkovsky