plexinc/papr

Make array elements required by default

Closed this issue · 3 comments

Example

Starting from the very simple example in the repository README, adding an array attribute pets:

import Papr, { schema, types } from 'papr';

const papr = new Papr();

const User = papr.model('users', schema({
  age: types.number(),
  firstName: types.string({ required: true }),
  lastName: types.string({ required: true }),
  pets: types.array(types.string())
}));

Rightfully, the generated type of pets will be (string | undefined)[] | undefined. Let's break it down a bit:
types.array doesn't specify { required: true } as a second parameter. Hence, the attribute is optional, and the type is Array | undefined.
In addition, the types.string() which corresponds to the elements of the array doesn't specify { required: true } which makes the type of elements inside the array string | undefined.

Suggestion

Can we assume { required: true } on the array elements specifier??

Discussion

I admit, the type system is working as designed, if you want to not have (string | undefined)[] then make sure to add { required: true } on the array element type. But I am curious if this is hurtful in more cases that it is helpful. Is there any value in storing undefined in a mongo array? ... idk 🤷

avaly commented

Technically we can do this.

From a consistency point of view, I feel this change would be weird. Reading the example schema you wrote above, I see that firstName and lastName are explicitly tagged as required, but not the pets value. It feels like the fact that those values are required will be a magic feature.

On the other hand, as a consumer of a papr model, you'd most likely work with the model's TypeScript type, not the schema, for 99% of the time.

I think the example looks even more interesting if you mark the array as required.

const User = papr.model('users', schema({
  age: types.number(),
  firstName: types.string({ required: true }),
  lastName: types.string({ required: true }),
  pets: types.array(types.string(), { required: true })
}));

The fact that [undefined] would be a valid value for pets would surprise me at a glance, even though as soon as you told me it was true I would understand why.

From a consistency point of view, I feel this change would be weird. Reading the example schema you wrote above, I see that firstName and lastName are explicitly tagged as required, but not the pets value. It feels like the fact that those values are required will be a magic feature.

Consider the following:

  • pets: types.array(types.string()) => type (string | undefined)[] | undefined // optional attribute with heterogeneous array.
  • pets: types.array(types.string( { required: true } )) => type string[] | undefined // optional attribute with homogeneous array.
  • pets: types.array(types.string(), { required: true } ) => type (string | undefined)[] // required attribute with heterogeneous array.
  • pets: types.array(types.string( { required: true } ), { required: true}) => type string[] // required attribute with homogeneous array.

The idea is to prevent the cases for heterogeneous arrays.