Discuss/document/eliminate/improve validateOptionsOnValidateValue

[samreid]

In #189 there was a lot of confusion about validateOptionsOnValidateValue. It is documented like so:

// By default, always check the provided options.  Can be turned off for cases where the same options are used
// repeatedly, such as in Property
validateOptionsOnValidateValue: true

// By default, check the options once in the constructor, not on each subsequent value validation, to improve
// performance in requirejs mode
validateOptionsOnValidateValue: false

@pixelzoom is this sufficient? Do you want further investigation about simplifying/renaming/removing this option?

[pixelzoom]

For the benefit of anyone else following this,validateOptionsOnValidateValue is an option to Validator.validate.

[pixelzoom]

Now that I understand this.... Here's a proposal.

Rename validateOptionsOnValidateValue to validateOptions.

Usages outside of Property would look like this, with validateOptions: true by default:

Validator.validate( options.initialNumberOfParticles, { 
  isValidValue: value => ...
} );

this.location = Validator.validate( options.location, { 
  valueType: Vector2 
} );

Usages inside of Property, or anything else that calls Validator.validate repeatedly, would look like this:

Validator.validate( someValue, { 
  ..., 
  validateOptions: false 
} );

In Property, I can't think of any reason to be validating the Validator options more than once. So remove validateOptionsOnValidateValue from Property's options, and change and relocatethis.validatorOptions assignment like this:

assert && Validator.validateOptions( this.validatorOptions );

this.validatorOptions = Validator.pickOptions( options, {

  // Validator options only need to be validate once, which we did above. 
  // Set to false to improve performance of subsequent Validator.validate calls. 
  validateOptions: false
} );

[zepumph]

So far we have been treating validateOptionsOnValidateValue as a field on a ValidatorDef.

What if instead we added an options arg to validate where we could specify this with the same pattern that we are used to. So we wouldn't need any hackary in Emitter and Property (like we have now). We would have:

validate( value, validator, { validateValidator: true })

and in Property and Emitter we could use the nested options pattern like:

{
  validateOptions: { validateValidator: false }
}

Because in those cases we wouldn't want to revalidate the validator each time the Property value changed, or each time the Emitter was emitted.

[samreid]

It may be even clearer to segregate these further. For instance, if the default is to validate the validation options, we could have:

validate(value,validator){
  validateValidator(validator);
  validateValue(value,validator);
}

Then sites that want to opt out of validating the validator each time (such as Property or Emitter) could use validateValue instead of validate.

[samreid]

I'm not sure what is better between the preceding two comments. @zepumph what do you think?

[zepumph]

I really like that all logic is islotated in ValidatorDef I think I prefer my recommendation in #207 (comment). What do you think? Here is a patch that exercises it:

Index: js/ValidatorDef.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ValidatorDef.js	(revision e9824fde1639d689700a3dad0c283d9688a6eb21)
+++ js/ValidatorDef.js	(date 1568935595220)
@@ -71,19 +71,6 @@
 
     // {function(PhetioType is a
     'phetioType'
-
-    /**************************************
-     * Additionally, validation will always check the validator itself.  However, for types like Property and Emitter,
-     * re-checking the validator every time the Property value changes or the Emitter emits wastes time. Hence cases like
-     * those can opt-out by specifying:
-     *
-     * validateOptionsOnValidateValue: false
-     *
-     * Note: this should not be a key in VALIDATOR_KEYS because the keys are reserved for checking the value itself,
-     * see implementation and usage of containsValidatorKey.  Our "definition" of a validator is the makeup of the keys
-     * above, validateOptionsOnValidateValue is more of a meta-option that is not for checking the value itself, but
-     * whether to check the validator at the same time.
-     *********************/
   ];
 
   const ValidatorDef = {
@@ -233,10 +220,19 @@
      */
     isValueValid( value, validator, options ) {
 
-      options = options || ASSERTIONS_FALSE;
+      options = _.extend( {
+
+        // {boolean} - By default validation will always check the validity of the  validator itself.  However, for types like
+        // Property and Emitter re-checking the validator every time the Property value changes or the Emitter emits
+        // wastes cpu. Hence cases like those can opt-out
+        validateOptions: true,
+
+        // if true, throw an assertion "instead" of waiting to return a boolean
+        assertions: false
+      }, options );
 
       // Use the same policy for whether to throw assertions when checking the validator itself.
-      if ( validator.validateOptionsOnValidateValue !== false && !axon.ValidatorDef.isValidValidator( validator, options ) ) {
+      if ( options.validateOptions !== false && !axon.ValidatorDef.isValidValidator( validator, options ) ) {
         assert && options.assertions && assert( false, 'Invalid validator' );
         return false;
       }
Index: js/Action.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Action.js	(revision e9824fde1639d689700a3dad0c283d9688a6eb21)
+++ js/Action.js	(date 1568935595224)
@@ -20,6 +20,8 @@
   const ValidatorDef = require( 'AXON/ValidatorDef' );
 
   // constants
+  const VALIDATE_OPTIONS_FALSE = { validateOptions: false };
+
   // Simulations have thousands of Emitters, so we re-use objects where possible.
   const EMPTY_ARRAY = [];
   assert && Object.freeze( EMPTY_ARRAY );
@@ -122,9 +124,6 @@
       for ( let i = 0; i < parameters.length; i++ ) {
         const parameter = parameters[ i ]; // metadata about a single parameter
 
-        assert && assert( parameter.validateOptionsOnValidateValue === undefined,
-          'Action sets its own validateOptionsOnValidateValue for each argument type'
-        );
         assert && assert( Object.getPrototypeOf( parameter ) === Object.prototype,
           'Extra prototype on parameter object is a code smell' );
 
@@ -147,11 +146,6 @@
           assert && assert( PARAMETER_KEYS.includes( key ), 'unrecognized parameter key: ' + key );
         }
 
-        // TODO: is this taking up too much memory? Does this create too much garbage? https://github.com/phetsims/axon/issues/257
-        parameters[ i ] = _.extend( {
-          validateOptionsOnValidateValue: false
-        }, parameter );
-
         // Changing after construction indicates a logic error.
         assert && Object.freeze( parameters[ i ] );
 
@@ -229,11 +223,11 @@
         );
         for ( let i = 0; i < this._parameters.length; i++ ) {
           const parameter = this._parameters[ i ];
-          validate( arguments[ i ], parameter );
+          validate( arguments[ i ], parameter, VALIDATE_OPTIONS_FALSE );
 
           // valueType overrides the phetioType validator so we don't use that one if there is a valueType
           if ( parameter.phetioType && !this._parameters.valueType ) {
-            validate( arguments[ i ], parameter.phetioType.validator );
+            validate( arguments[ i ], parameter.phetioType.validator, VALIDATE_OPTIONS_FALSE );
           }
         }
       }
Index: js/validate.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/validate.js	(revision e9824fde1639d689700a3dad0c283d9688a6eb21)
+++ js/validate.js	(date 1568935385077)
@@ -17,15 +17,16 @@
    * If assertions are enabled, assert out if the value does not adhere to the validator. No-op without assertions.
    * @param {*} value
    * @param {ValidatorDef} validator
+   * @param {Object} [options] - see ValidatorDef.isValueValid
    * @returns {*} - returns the input value for chaining
    * @public
    */
-  const validate = ( value, validator ) => {
+  const validate = ( value, validator, options ) => {
 
     if ( assert ) {
 
       // Throws an error if not valid
-      ValidatorDef.isValueValid( value, validator, { assertions: true } );
+      ValidatorDef.isValueValid( value, validator, _.extend( { assertions: true }, options ) );
     }
     return value;
   };
Index: js/Property.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Property.js	(revision e9824fde1639d689700a3dad0c283d9688a6eb21)
+++ js/Property.js	(date 1568935595217)
@@ -19,6 +19,9 @@
   const validate = require( 'AXON/validate' );
   const ValidatorDef = require( 'AXON/ValidatorDef' );
 
+  // constants
+  const VALIDATE_OPTIONS_FALSE = { validateOptions: false };
+
   // variables
   let globalId = 0; // autoincremented for unique IDs
 
@@ -46,10 +49,6 @@
         // cycles may pollute the data stream. See https://github.com/phetsims/axon/issues/179
         reentrant: false,
 
-        // By default, check the options once in the constructor, not on each subsequent value validation, to improve
-        // performance in requirejs mode
-        validateOptionsOnValidateValue: false
-
         // Property also supports validator options, see ValidatorDef.VALIDATOR_KEYS.
 
       }, options );
@@ -136,7 +135,7 @@
         }
         ValidatorDef.validateValidator( validator );
 
-        this.validate = value => validate( value, validator );
+        this.validate = value => validate( value, validator, VALIDATE_OPTIONS_FALSE );
 
         // validate the initial value
         this.validate( value );

[samreid]

That looks great, thanks!

[zepumph]

Excellent! Committed above. @pixelzoom thank you so much for this solution. I'm really happy with it. It is clean and precise, unlike the last one. Closing