required attributes misleading

[cdaringe]

hi all:

i'm curious how everyone has internalized required attributes. the docs make note of how they behave, but do we actually believe this is the correct behavior?

example: if my model has a prop _rev that is as required string, and I instantiate new MyModel() without any attrs, I expect &-state to error. In actuality, &-state eventually returns the default for a string definition, thus _rev is initially ''. @HenrikJoreteg originally put in _verifyRequired, which is dead code in &-state now and doesn't accomplish what I suggest, but seems as though it may have been the intent.

id like to hear folks opinions. I want instantiation assertion, along with .set assertion i already get from the lib when changing my state.

thanks!

[cdaringe]

for the time being, i'm experimenting with

    initialize: function(attrs) {
        for (var def in this._definition) {
            var defDfl = this._definition[def].default;
            if (this._definition[def].required &&
                (!defDfl || (typeof defDfl === 'function' && !defDfl())) &&
                (attrs[def] === undefined || attrs[def] === null)) {
                throw new ReferenceError('expected value for attr `' + def + '`. received ' + attrs[def]);
            }
        }
        Model.prototype.initialize.apply(this, arguments);
    }

[wraithgar]

Ah yes, this old peach. I've personally never been 100% satisfied w/ the way props are defined, thinking "just one more major version tweak and I think we likely have it". There is still an open discussion ticket here #48

Ultimately I think we should decide "what does required actually mean, if anything" and use that to iron out how we define props. But that's just my opinion.

[wraithgar]

Also related: #55

[pgilad]

I think required means that when initializing a model - these props must be non - undefined. That's definitely not the way it's currently set.

[wraithgar]

And if they are undefined and there is a default? Should required and default be mutually exclusive?

[cdaringe]

yikes, can't believe i didn't find those oldies-but-goodies

And if they are undefined and there is a default? Should required and default be mutually exclusive?

i would say they must be able to coexist. if you set a prop to undefined that is required, then it semantically and practically violates any meaning that required inferred. however, there's no reason why a default shouldn't be able to provide an initial value! therefore, "let me the user provide an initial value, and assert that there is forever a value that meets the type requirement or allowNull requirement"

I think, @wraithgar, your notes #48 echo this

[pgilad]

I think we should act like mongoose on model schema. A field can be both required and have a default (in which case the default gets set if undefined).

In that case - required throws in 3 situations:

1. Model init with required=true and no default
2. Field unset with required=true and no default
3. Field is set to undefined (and we don't care about default here)

[cdaringe]

@pgilad i like it. i'm trying to poke holes in it for hardening sake, but im comin' up short!

[codepunkt]

What @pgilad said. I actually just stumbled over this, because i want a string that throws when undefined - which doesn't even work using test because values are only tested when defined.

[cmazakas]

I'm so glad other people are talking about this! I literally came here to write about just this!

To me, setting a property to "required" means that the constructor should throw which is perfectly amazing semantic behavior considering constructors don't have return values and it'd be nice to know if someone was instantiating an object without all the "required" parameters.

[orenmizr]

Sounds like you want to change required to "fallback to default" and make it a prerequisite for valid initialization. might be a nice change...

[cmazakas]

I think it'd be a super nice change. It's not like the current functionality of 'required' is bad. That should stay. But I was certainly coding under the impression that the constructor would throw if I (or anyone else on my team) had forgotten to pass the proper parameters to the state.

[cdaringe]

I would love if someone (&-js community member perhaps?? :) ) had some time to hack a PR together for this. I think that the issue is acknowledged widely enough, and that there's good enough consensus on what needs to change. Specifically, @pgiliad outlined a great proposal that supports this issue's original problem statement , supports @wraithgar's marks in #48, as well as all others' follow ups in the thread. With that, let us welcome the issue up some PR activity :)

[janpaul123]

For those reading this, we're using this workaround for now at @remix until we migrate to Redux:

function verifyRequiredProps(props, model) {
  Object.keys(props).forEach((propName) => {
    if (props[propName].required && !model.attributes[propName]) {
      throw new Error(`Missing required prop "${propName}"`);
    }
  });
}

Which we then call in initialize like this: verifyRequiredProps(props, this); (you have to pull out props into a variable).

[gannons]

Seems this issue is still open. Got caught out by it.