support for valueType:Array

[pixelzoom]

Noted during Graphing Quadratics code review phetsims/graphing-quadratics#43, when working on this checklist item:

• Assertions should be used appropriately and consistently. Type checking should not just be done in code comments. Use Array.isArray to type check an array.

"Use Array.isArray to type check an array" does not appear to be the case in Property validation.

Example in GRAPHING_QUADRATICS/GQModel:

// Options for {Property.<Quadratic[]>}
const optionsPropertyQuadraticArray = {
  valueType: Array,
  isValidValue: array => _.every( array, function( value ) { return value instanceof Quadratic; } )
};

Property's valueType option is handled by assertValueType.js, which contains no special handling for Array. So valueType: Array gets checked using instanceof, not Array.isArray.

assertValueType.js should either be fixed to handle Array properly, or an assertion should be added to disallow valueType: Array. If the latter, then the above code would need to change to:

// Options for {Property.<Quadratic[]>}
const optionsPropertyQuadraticArray = {
  isValidValue: array => Array.isArray( array ) &&
    _.every( array, function( value ) { return value instanceof Quadratic; } )
};

@samreid @jonathanolson opinions? Keep in mind that we're discussing unifying the various validation options in #189.

High priority because this affects Graphing Quadratics.

[pixelzoom]

To fix assertValueType.js, add this after line 34:

      else if ( valueType === Array ) {
        assert( Array.isArray( value ), 'value should be an Array, value=' + value );
      }

Does that seem like an acceptable solution?

[pixelzoom]

The above passes lint. But WebStorm is complaining about ( valueType === Array ), with message "Binary operation argument type Function is not compatible with type ArrayConstructor". Can anyone translate that for me? Something to be concerned about?

[pixelzoom]

GRAPHING_QUADRATICS/GQModel appears to be the only place where valueType: Array is used.

[pixelzoom]

Since I don't have time to be a trailblazer here, I changed GRAPHING_QUADRATICS/GQModel to handle the Array.toArray call, i.e.:

// Options for {Property.<Quadratic[]>}
const optionsPropertyQuadraticArray = {
  isValidValue: array => Array.isArray( array ) &&
    _.every( array, function( value ) { return value instanceof Quadratic; } )
};

Removing high priority since this no longer affects Graphing Quadratics.

Leaving this issue open to decide how to handle in general.

[samreid]

The isValidValue solution sounds great for this case, and the long term solution will depend on what is decided in #189

[samreid]

On hold until #189 is decided.

[zepumph]

I think #189 is far enough along to bring this back up for discussion. I have two potential proposals:

1. This approach assumes that we only support a homogeneous array: valueType: ['number']. Thus we could check if Array.isArray(options.valueType) then we assert that it only has one item in it, and we treat that as the valueType for all values in the array.
   • This approach is a bit cryptic, but it does potentially allow support in the future for heterogeneous arrays.
   • This approach is nice because it doesn't require giving a valueType AND isValidValue.
2. We could have Validator.js have a static function in which you give it a value type ('number' or Node etc.), and it returns the options needed to pass to Validator.validate in order to validate the array. A method called Validator.getArrayValidationOptions() or something.

[samreid]

Writing down some options.

// Proposal 1 from preceding comment
new Property(myArray,valueType:['number'])
new Property(myArray,valueType:[Vector2])

// Proposal 2 from preceding comment
new Property(myArray,isValidValue: Validator.getArrayValidator('number'));

// Option 3: checking that it is an array and all values match a spec, using Validator and existing support
new Property(myArray,{isValidValue: v=>Array.isArray(v) && _.every(v, element=>Validator.validate(element,{valueType: 'number'})}

// Option 4: new key type
new Property(myArray,{arrayElementType: 'number'});
new Property(myArray,{arrayElementType: Vector2});

1 and 4 seem nice. In #189 (comment) we discussed keeping typeof and instanceof bound together in valueType and now we're talking about throwing another type in there. Not sure if it is clever or messy. What if we had primitiveType: 'boolean' vs valueType: Vector2 vs arrayElementType: 'number'? Or maybe we should consider what was done for array types in Query String Machine--in that case it would be something like:

// Option 5: nested validator for arrays
new Property(myArray, {arrayElementType:{validValues: [1,2,3]}} // array of values 1,2,3 only
new Property(myArray, {arrayElementType:{isValidValue: x => x>0;}} // array of positive numbers

[pixelzoom]

My assumption in the solution proposed in #195 (comment) was that checking that the value is an Array is sufficient. Do you really feel that we need to check that all of the elements in the Array are of some homogenous type? I'm not convinced that's needed or worth the effort.

[pixelzoom]

And what if you have an Array of Arrays of Arrays of... ? Do you propose to drill all the way down to test the types of the leaf elements?

[jonathanolson]

Do you really feel that we need to check that all of the elements in the Array are of some homogenous type? I'm not convinced that's needed or worth the effort.

I've found that helpful in the past in a few cases, usually doing assert && array.forEach( element => assert( element instanceof X ) ).

No strong feelings, but it seems if Validator can see the Array type and convert the check to Array.isArray that seems nice.

[samreid]

And what if you have an Array of Arrays of Arrays of... ? Do you propose to drill all the way down to test the types of the leaf elements?

I don't want to choose a validation strategy that makes that impossible. Option 5 from above makes this possible via:

// Array of arrays of positive values
{arrayElementType:{arrayElementType:{isValidValue: v=> v>0}}}

My assumption in the solution proposed in #195 (comment) was that checking that the value is an Array is sufficient.

That is already supported via {isValidValue: v=>Array.isArray(v)}. We should either (a) assert that the user didn't try to put {valueType: 'array'} or {valueType: Array}, or (b) we could add support for those. In the preceding comment, it sounds like @jonathanolson prefers (b).

[pixelzoom]

I really think that something like:

new Property( ..., {
  valueType: Array,
  isValidValue: value => // predicate to validate what's in Array
} );

... would be sufficient to handle validation of Arrays (and Arrays of Arrays, etc.) If you really want to incur the cost/complexity of building that Array-specific support into Validator, go for it. But I think the payoff will be minimal.

[zepumph]

I think that valueType: Array, is insufficient on it's own. I too like that approach, but we would need to add this line into Validator.validate:

if ( options.valueType ) {
          const valueType = options.valueType;
// start new block **************************************************
          if ( valueType === Array){
                assert( Array.isArray( value ));
          }
// end new block **************************************************
          if ( typeof valueType === 'string' ) { // primitive type
            assert( typeof value === valueType, 'value should have typeof ' + valueType + ', value=' + value );
          }
          else if ( typeof valueType === 'function' ) { // constructor
            assert( value instanceof valueType, 'value should be instanceof ' + valueType.name + ', value=' + value );
          }
        }

[pixelzoom]

That's pretty much what I proposed in #195 (comment).

[zepumph]

arrayElementType

I think that naming it elementType would be more general, because perhaps it could work for another composite type? Maybe that's too vague.

I think there are two leading ideas about how we go, please correct me if I am mistaken:

1. Use of valueType: Array with isValidValue:

new Property( ..., {
 valueType: Array,
 isValidValue: value => value.forEach((element)=>typeof element === 'string'); // predicate to validate what's in Array
} );

2. Use of valueType:Array with new key arrayElementType (maybe elementType) where the value of arrayElementType is of type {ValidatorOptions}(or maybe it takes the place ofvalueType`, not sure that was decided):

new Property( ..., {
  valueType: Array,
  arrayElementType: {valueType: 'string'}
} );

I think that including valueType: Array is very explicit and makes things clear and nice. I think I prefer option 2 because it feels like Validator is doing more work for us with little downside. Perhaps there is a downside in the proliferation of more ValidatorOptions.

It's good to note that this has always been a problem for Property validation, the issue has just moved over to being Validator's problem since #189.

Do people like option 1 or 2 or another thing better?

[pixelzoom]

Option 1 seems sufficient to me, as I said in #195 (comment).

[samreid]

@zepumph @chrisklus and I added support for valueType: Array in the preceding commit. This does not preclude us from checking element types in the future.

[samreid]

I think valueType: Array will be sufficient for now. If a client needs to check values, they can additionally use isValidValue. In the future, we have the option to add direct support for checking individual element types, but that doesn't seem necessary now. @zepumph is that satisfactory? Can this issue be closed?

[zepumph]

Yes I think this is most efficient and best way forward right now.

[zepumph]

This came up again as part of #227. Reopening.

As discussion with @pixelzoom and @samreid, it seems like we should make a new validator key like;

{
  arrayElementType: Type
}

which will basically do:

{
         valueType: Array,

        // {TermCreator[]}
        isValidValue: array => {
          return _.every( array, value => value instanceof TermCreator )
        }
      }

[pixelzoom]

The other common case I’m running into is {SomeType|null}. And I’m having to write this type of validator:

{ 
  // {SomeType|null}
  isValidValue: value => ( value instanceof SomeType || value === null ) 
}

[zepumph]

The other common case I’m running into is {SomeType|null}. And I’m having to write this type of validator

see #228

[zepumph]

arrayElementType was added in the above commit. @samreid please review this when you review #228.

[samreid]

Everything looks great, and the unit tests have good coverage, nice work! Thanks. Closing.