add support for multiple valueTypes

[zepumph]

This came up during #227.

It is very annoying to have to do:

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

each time.

Brainstorming:
new key?
{ nullOrValueType: Event }

function on ValidatorDef?
ValidatorDef.getNullOrTypeValidator( Event )

[pixelzoom]

Allow valueType to be an array of possible value types. E.g.:

validators: {
  { valueType: [ Event, null ] }
}

[zepumph]

I like that! Is it confusing that that key is singular?

[pixelzoom]

I don't think it's confusing. "The value type can be Event or null."

[zepumph]

Sounds good! I can implement.

[samreid]

Sounds great, nice idea!

[pixelzoom]

A couple of things about valueType: [ Event, null ]...

(1) null is not a type, it's a value.
(2) It doesn't work for type-specific Properties like Vector2Property.

[samreid]

(1) null is not a type, it's a value.

So true, but I think we can make an exception since [Event,null] is nice and readable (unless we think of something nicer).

(2) It doesn't work for type-specific Properties like Vector2Property.

True, but I think type-specific properties should generally not support null, and in going through the many cases in phetsims/dot#88, it seemed like most/all new Vector2Property were initialized with a non-null Vector2, so that pattern may not be too frequent. In fact, earlier today I documented it as such:

Convenience subtype of Property that takes only Vector2 values (no null values)

Cases that do require "or null" could either instantiate raw Property and handle the options directly, or instantiate Vector2Property and specify options for valueType and phetioType. But like I said, I hope and expect that to be on the rare side.

[samreid]

#231 was closed as duplicated, but it was marked for developer meeting discussion. Should we mark this for developer meeting?

[pixelzoom]

@samreid said:

So true, but I think we can make an exception since [Event,null] is nice and readable (unless we think of something nicer)

Sounds good.

Cases that do require "or null" could either instantiate raw Property and handle the options directly

That sounds reasonable.

or instantiate Vector2Property and specify options for valueType and phetioType.

Type-specific Properties should not be able to change their type. BooleanProperty handles this correctly. Vector2Property handles this incorrectly.

But like I said, I hope and expect that to be on the rare side.

Assuming all such Properties have a type expression documented (which I doubt)...
With string search, I see:

165 occurrences of regex Property.<.*\|null,
3 {Property.<Vector2|null>}
26 {Property.<number|null>}
1 {Property.<string|null>}
0 {Property.<boolean|null>}

[zepumph]

Alright then it sounds like the consensus is to have valueType support an array (technically making it value types) and allowing one of those two Properties to be null. Right now I feel like it is nice to restrict this only down to what is currently needed. We can open another issue to discuss if we want to allow valueType: [Node, 'number'].

So the restrictions will be as follows:

• valueType now allows a value to be an Array
• if it is an array, then it can only be two values
• if it is an array, then one of the two values must be null.

[zepumph]

Here is a patch I made brainstorming this idea, honestly I sorta hate this. Perhaps it would look cleaner to not put the above stipulations on the array, and just support any number of valueTypes. I think that if there was a loop instead of my logic it would simplify things supstantially

Index: js/ValidatorDef.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ValidatorDef.js	(revision 69df9c71a0597d06adb5badeb4994799ff42e2a4)
+++ js/ValidatorDef.js	(date 1552118289202)
@@ -35,13 +35,16 @@
   const ASSERTIONS_FALSE = { assertions: false };
   const ASSERTIONS_TRUE = { assertions: true };
 
+  const isArrayWithNullAsValue = valueType => Array.isArray( valueType ) && valueType.length === 2 && valueType.indexOf( null ) >= 0;
+
   // Key names are verbose so this can be mixed into other contexts like AXON/Property. `undefined` and `null` have the
   // same semantics so that we can use this feature without having extend and allocate new objects at every validation.
   const VALIDATOR_KEYS = [
 
-    // {function|string|null} type of the value.
+    // {function|string|null|Array} type of the value.
     // If {function}, the function must be a constructor.
     // If {string}, the string must be one of the primitive types listed in TYPEOF_STRINGS.
+    // If {Array}, used to accept {Type|null}, one value should be an aforementioned value, and the other should be "null"
     // Unused if null.
     // Examples:
     // valueType: Vector2
@@ -96,9 +99,10 @@
       const valueType = validator.valueType;
       if ( !( typeof valueType === 'function' ||
               typeof valueType === 'string' ||
+              ( isArrayWithNullAsValue( valueType ) ) ||
               valueType === null ||
               valueType === undefined ) ) {
-        assert && options.assertions && assert( false, `valueType must be {function|string|null}, valueType=${valueType}` );
+        assert && options.assertions && assert( false, `valueType must be {function|string|null|Array}, valueType=${valueType}` );
         return false;
       }
 
@@ -198,6 +202,19 @@
           assert && options.assertions && assert( false, `value should have been an array, value=${value}` );
           return false;
         }
+        else if ( isArrayWithNullAsValue( valueType ) ) {
+
+          // get the other index, there must be 2
+          const nonNullIndex = valueType.indexOf( null ) + 1 % 2;
+
+          if ( !( value === null || // either null or
+
+                  // test value against other valueType
+                  ValidatorDef.isValueValid( value, { valueType: valueType[ nonNullIndex ] }, options ) ) ) {
+            assert && options.assertions && assert( false, `value should have been an array, value=${value}` );
+            return false;
+          }
+        }
         else if ( typeof valueType === 'function' && !( value instanceof valueType ) ) { // constructor
           assert && options.assertions && assert( false, `value should be instanceof ${valueType.name}, value=${value}` );
           return false;

[samreid]

Perhaps it would look cleaner to not put the above stipulations on the array, and just support any number of valueTypes.

My instinct is that we would not want to see client usage like valueTypes: [OneType, ACompletelyDifferentType] (whether the key is singular valueType or plural valueTypes). I'm not bothered by complex implementation--clarity and readability at the numerous client sites is worth it.

[pixelzoom]

@samreid said:

My instinct is that we would not want to see client usage like valueTypes: [OneType, ACompletelyDifferentType]

I can't recall using a Property that takes a value that can be of multiple types. But if it's a legitimate use that passes review, I don't see why a client shouldn't be able to do this. The implementation should not make it impossible to support this. And it should be very easy to support this, since we're just iterating over an array.

[pixelzoom]

I said:

I can't recall using a Property that takes a value that can be of multiple types.

Didn't take long to contradict that statement :) Property.<Color|string|null> is technically a Property that takes a value of multiple types. We've invented ColorDef as a way of specifying that in some situations (like type expressions), but as we've seen with ColorProperty, it's complicated.

[samreid]

That's true, but I wonder if in practice we would want to use:

isValidValue: value => value===null || ColorDef.isColorDef(value)
// Reuses ColorDef properly, but verbose because it adds null check

or

valueType:[Color,string,null]
// Not a complete use of ColorDef, because it doesn't check for Property.<Color|string>

I guess I'm trying to narrow down whether valueType:[OneType,ADifferentType] is a code smell or necessary evil. The question about Color|string|null doesn't convince me that it's necessary, but maybe another case will. But even if we see another case in practice that is coded like valueType:[OneType,ADifferentType], wouldn't we be interested in reimplementing that so it is only one type?

[pixelzoom]

@samreid said:

... I guess I'm trying to narrow down whether valueType:[OneType,ADifferentType] is a code smell or necessary evil.

A dynamically-typed validator is (in general) neither of those. It's an important feature of Property and other PhET building blocks, courtesy of Javascript. Whether it's a code smell depends on context.

The question about Color|string|null doesn't convince me that it's necessary, but maybe another case will.

How to best implement ColorProperty depends on what we decide in phetsims/scenery#948. Regardless of what we decide there, that shouldn't dictate whether we implement support for dynamically-typed validators, because validators are not specific to Property. And I don't feel that we need a second example as justification.

But even if we see another case in practice that is coded like valueType:[OneType,ADifferentType], wouldn't we be interested in reimplementing that so it is only one type?

I'm not convinced about the ColorDef pattern, and whether it should be applied more widely than it currently is. And I certainly don't want to be forced to use that pattern whenever I need a dynamically-typed validator. For example (hypothetical), I don't want to have to create ToolDef just to support this:

// The tool that the user is interacting with right now.
const toolProperty = new Property( ..., {
  valueTypes: [ VoltMeter, OhmMeter, MeasuringTape, null ]
} );

So my vote is to implement support for dynamically-typed validators, because:

(1) PhET does (and will continue to) leverage Javascript's dynamic typing.
(2) When and whether to use dynamic types is context specific, and is most appropriate to address in code review.
(3) Validators can be widely used. They are not specific to Property, Emitter, etc.
(4) My crystal ball doesn't indicate that valueType: [ Type1, Type2, ... ] should not be supported.
(5) It will cost less to implement now than to discuss further or add in the future.

[samreid]

Thanks for discussing that, I'm convinced to move forward with valueType: [ Type1, Type2, ... ] as an alternative to valueType: Type. @zepumph want to implement? Or let me know if I should.

[zepumph]

I made a bit of progress on this today, though I didn't get to a commit point:

One thing of note is that I can't get assertions to work yet within an array, so we may need to create our own assertions where isValueValidValueType is called for the array.

Index: js/ValidatorDefTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ValidatorDefTests.js	(revision b96d811b1967f97ef115bb5e3f92fbdecfd7600f)
+++ js/ValidatorDefTests.js	(date 1563499646355)
@@ -12,6 +12,7 @@
   const Enumeration = require( 'PHET_CORE/Enumeration' );
   const ValidatorDef = require( 'AXON/ValidatorDef' );
   const validate = require( 'AXON/validate' );
+  const Node = require( 'SCENERY/nodes/Node' );
 
   QUnit.module( 'Validator' );
 
@@ -63,6 +64,20 @@
     assert.ok( !ValidatorDef.isValidValidator( { valueType: 'blaradysharady' } ), 'invalid valueType string' );
     assert.ok( ValidatorDef.isValidValidator( { isValidValue: () => {} } ), 'isValidValue is a function' );
     assert.ok( !ValidatorDef.isValidValidator( { isValidValue: 'hi' } ), 'isValidValue should not be string' );
+
+    assert.ok( ValidatorDef.isValidValidator( { valueType: null } ), 'null is valid' );
+    assert.ok( ValidatorDef.isValidValidator( { valueType: [ 'number', null ] } ), 'array of null and number is valid' );
+    assert.ok( ValidatorDef.isValidValidator( { valueType: [ 'number', null, Node ] } ), 'array of null and number is valid' );
+    assert.ok( !ValidatorDef.isValidValidator( { valueType: [ 'numberf', null, Node ] } ), 'numberf is not a valid valueType' );
+
+  } );
+
+  QUnit.test( 'Test valueType: {Array}', assert => {
+    assert.ok( ValidatorDef.isValueValid( null, { valueType: null } ), 'null is valid' );
+    assert.ok( ValidatorDef.isValueValid( 7, { valueType: [ 'number', null ] } ), '7 is valid for null and number' );
+    assert.ok( ValidatorDef.isValueValid( null, { valueType: [ 'number', null ] } ), 'null is valid for null and number' );
+    assert.ok( ValidatorDef.isValueValid( new Node(), { valueType: [ 'number', null, Node ] } ), 'Node is valid' );
+    assert.ok( !ValidatorDef.isValueValid( 'hello', { valueType: [ 'number', null, Node ] } ), 'string not valid' );
   } );
 
   QUnit.test( 'Test valueType: {Enumeration}', assert => {
Index: js/ValidatorDef.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ValidatorDef.js	(revision b96d811b1967f97ef115bb5e3f92fbdecfd7600f)
+++ js/ValidatorDef.js	(date 1563500031461)
@@ -15,7 +15,7 @@
  *
  * A validator that accepts any Object:
  * { valueType: Object }
- * 
+ *
  * A validator that accepts Enumeration values:
  * { valueType: MyEnumeration }
  * or
@@ -48,11 +48,14 @@
     // {function|string|null} type of the value.
     // If {function}, the function must be a constructor.
     // If {string}, the string must be one of the primitive types listed in TYPEOF_STRINGS.
+    // If {null|undefined}, the value must be null (which doesn't make sense until the next line of doc)
+    // If {Array.<string|function|null|undefined>}, each item must be a legal value as explained in the above doc
     // Unused if null.
     // Examples:
     // valueType: Vector2
     // valueType: 'string'
-    // valueType: 'number'
+    // valueType: 'number',
+    // valueType: [ 'number', null ]
     'valueType',
 
     // {*[]|null} valid values for this Property. Unused if null.
@@ -88,7 +91,7 @@
      * @returns {boolean}
      * @public
      */
-    isValidValidator: ( validator, options ) => {
+    isValidValidator( validator, options ) {
 
       options = options || ASSERTIONS_FALSE;// Poor man's extend
 
@@ -100,22 +103,19 @@
         return false;
       }
 
-      const valueType = validator.valueType;
-      if ( !( typeof valueType === 'function' ||
-              typeof valueType === 'string' ||
-              valueType instanceof Enumeration ||
-              valueType === null ||
-              valueType === undefined ) ) {
-        assert && options.assertions && assert( false,
-          `valueType must be {function|string|Enumeration|null|undefined}, valueType=${valueType}` );
-        return false;
-      }
-
-      // {string} valueType must be one of the primitives in TYPEOF_STRINGS, for typeof comparison
-      if ( typeof valueType === 'string' ) {
-        if ( !_.includes( TYPEOF_STRINGS, valueType ) ) {
-          assert && options.assertions && assert( false, `valueType not a supported primitive types: ${valueType}` );
-          return false;
+      if ( validator.hasOwnProperty( 'valueType' ) ) {
+        const valueType = validator.valueType;
+
+        // if every item is not valid, then return false
+        if ( Array.isArray( valueType ) ) {
+          if ( !_.every( valueType.map( typeInArray => ValidatorDef.validateValueType( typeInArray, options ) ) ) ) {
+            return false;
+          }
+        }
+        else if ( valueType ) {
+          if ( !ValidatorDef.validateValueType( valueType, options ) ) {
+            return false;
+          }
         }
       }
 
@@ -145,6 +145,34 @@
           }
         }
       }
+      return true;
+    },
+
+    /**
+     * @private
+     * @param valueType
+     * @param {Object} options - requried, options from isValidValidator
+     * @returns {boolean} - true if valid
+     */
+    validateValueType( valueType, options ) {
+      if ( !( typeof valueType === 'function' ||
+              typeof valueType === 'string' ||
+              valueType instanceof Enumeration ||
+              valueType === null ||
+              valueType === undefined ) ) {
+        assert && options.assertions && assert( false,
+          `valueType must be {function|string|Enumeration|null|undefined}, valueType=${valueType}` );
+        return false;
+      }
+
+      // {string} valueType must be one of the primitives in TYPEOF_STRINGS, for typeof comparison
+      if ( typeof valueType === 'string' ) {
+        if ( !_.includes( TYPEOF_STRINGS, valueType ) ) {
+          assert && options.assertions && assert( false, `valueType not a supported primitive types: ${valueType}` );
+          return false;
+        }
+      }
+
       return true;
     },
 
@@ -153,7 +181,7 @@
      * @param {ValidatorDef} validator
      * @public
      */
-    validateValidator: validator => {
+    validateValidator( validator ) {
       if ( assert ) {
 
         // Specify that assertions should be thrown if there are problems during the validation check.
@@ -199,29 +227,58 @@
       // See https://github.com/phetsims/axon/issues/201
       if ( validator.hasOwnProperty( 'valueType' ) ) {
         const valueType = validator.valueType;
-        if ( typeof valueType === 'string' && typeof value !== valueType ) { // primitive type
-          assert && options.assertions && assert( false, `value should have typeof ${valueType}, value=${value}` );
-          return false;
-        }
-        else if ( valueType === Array && !Array.isArray( value ) ) {
-          assert && options.assertions && assert( false, `value should have been an array, value=${value}` );
-          return false;
-        }
-        else if ( valueType instanceof Enumeration && !valueType.includes( value ) ) {
-          assert && assert( false, 'value is not a member of Enumeration ' + valueType );
-          return false;
-        }
-        else if ( typeof valueType === 'function' && !( value instanceof valueType ) ) { // constructor
-          assert && options.assertions && assert( false, `value should be instanceof ${valueType.name}, value=${value}` );
-          return false;
-        }
+        if ( Array.isArray( valueType ) ) {
+
+          // Only one will be valid, so error out if none of them returned valid
+          // Hard code assertions false because most will fail.
+          if ( !_.some( valueType.map( typeInArray => ValidatorDef.isValueValidValueType( value, typeInArray, ASSERTIONS_FALSE ) ) ) ) {
+            return false;
+          }
+        }
+        else if ( valueType ) {
+          if ( !ValidatorDef.isValueValidValueType( value, valueType, options ) ) {
+            return false;
+          }
+        }
+      }
+      if ( validator.hasOwnProperty( 'validValues' ) && validator.validValues.indexOf( value ) === -1 ) {
+        assert && options.assertions && assert( false, `value not in validValues: ${value}` );
+        return false;
+      }
+      if ( validator.hasOwnProperty( 'isValidValue' ) && !validator.isValidValue( value ) ) {
+        assert && options.assertions && assert( false, `value failed isValidValue: ${value}` );
+        return false;
+      }
+      return true;
+    },
+
+    /**
+     * @param {Object|null} value
+     * @param {string|function|null|undefined} valueType - see above definition, Array is not allowed in this method
+     * @param {Object} options - not optional, should be passed in from isValidValue
+     * @returns {boolean} - whether the value is a validType
+     * @throws {Error} assertion error if not valid and options.assertions is true
+     * @private
+     */
+    isValueValidValueType( value, valueType, options ) {
+      if ( typeof valueType === 'string' && typeof value !== valueType ) { // primitive type
+        assert && options.assertions && assert( false, `value should have typeof ${valueType}, value=${value}` );
+        return false;
+      }
+      else if ( valueType === Array && !Array.isArray( value ) ) {
+        assert && options.assertions && assert( false, `value should have been an array, value=${value}` );
+        return false;
+      }
+      else if ( valueType instanceof Enumeration && !valueType.includes( value ) ) {
+        assert && assert( false, 'value is not a member of Enumeration ' + valueType );
+        return false;
+      }
+      else if ( typeof valueType === 'function' && !( value instanceof valueType ) ) { // constructor
+        assert && options.assertions && assert( false, `value should be instanceof ${valueType.name}, value=${value}` );
+        return false;
       }
-      if ( validator.hasOwnProperty( 'validValues' ) && validator.validValues.indexOf( value ) === -1 ) {
-        assert && options.assertions && assert( false, `value not in validValues: ${value}` );
-        return false;
-      }
-      if ( validator.hasOwnProperty( 'isValidValue' ) && !validator.isValidValue( value ) ) {
-        assert && options.assertions && assert( false, `value failed isValidValue: ${value}` );
+      if ( valueType === null && value !== null ) {
+        assert && options.assertions && assert( false, `value should be null, value=${value}` );
         return false;
       }
       return true;

[zepumph]

In the above commits, I added support for multiple valueTypes, via a list. This went pretty smoothly, but it added complexity to both validator validating, and value validating. Both cases were treated basically the same, in which I factored out validation for a single valueType into a method that I could call once for each element in the array. The main difference is that when validating the validator valueTypes themselves, we want every one to be valid (notice the usage of _.every), but for value validation, we only expect one (hence the usage of _.some), and want to fail out if none pass validation.

The only other hickup (ish) was that in isValueValid, we had to always options to make sure assertions were not on. This is to support the _.some mentality spoken of above. As a result I added a more general assertion for if a value fails a list of valueTypes.

I also went through and converted usages I could find, largely with the regex isValidValue.*null.

@samreid please review.

[samreid]

Everything looks really great. Can if ( options.validateValidator !== false be changed to if ( options.validateValidator? It seems so but I thought it best to double check. After that, I think this issue can be closed.

[zepumph]

good call!