Use validator instead of assertInstanceOf

[samreid]

From #204. Now that we have an improved pattern for validation for IO type values, we should leverage that instead of having a parallel but different way of validating via assertInstanceOf.

[samreid]

Proposed changes:

ObjectIO constructor:

    // Use the validator defined on the constructor to make sure the instance is valid
    assert && validate( instance, this.constructor.validator );

phetioInherit:

assert && assert( staticProperties, 'static properties must be defined' );
    assert && assert( staticProperties.validator, 'validator must be provided' );
    ValidatorDef.validateValidator( staticProperties.validator );

Then we can add validator entries to IO types, and remove the type check in the constructors.

We would need a nice syntax to check for toStateObject/fromStateObject as well, not sure what that will be.

[zepumph]

Then we can add validator entries to IO types

Yes the code should have that. I thought that, I thought that this was added in #204 though. Everything recommended above sounds great.

[samreid]

I don't think this approach will work because of a circularity problem in loading these modules. Typically we reference the IO type from the core type, and use namespace to get the core type from the IO type. However, IO type is loaded first (since it is a dependency) and the core type doesn't exist at that time.

[zepumph]

Why would it not work to consistently use the name space to reference the core type from the io type? Since all the usages are nested in the io type constructor, which won't be called until the core type has been loaded to the name space?

[samreid]

@zepumph and I discussed it, and considered lazily creating the validators. That is problematic because there are several usage sites. Then @zepumph wrote:

Michael Kauzmann [11:16 AM]
you could also make it so that all typeio validators have to use isValidValue instead of other validator keys
that would create the proper closure too.
(I think)

Sam Reid [11:19 AM]
nice! That may be better. I’m seeing usages in toStateObject/fromStateObject which may need to be guarded like we did in ObjectiO constructor.

[samreid]

using isValidValue where namespaces were used seems to be working nicely. Local phet-io aqua is passing. Local aqua is good so far. I'll notify slack:

Large commit incoming to use validator for IO types, see #226. This will be around 90 files across around 20 repos.

[samreid]

Sam Reid [11:50 AM]
Pushed, please pull and report problems here or in #226

Over to @zepumph for review.

[samreid]

Also, please search through the repos for https://github.com/phetsims/axon/issues/226 which indicates some places where it's unclear what to put for these validators.

[zepumph]

I removed one todo. I also am stumped about the other two.

[zepumph]

Everything else looks good though.

[zepumph]

Oh! I'm also wanting to discuss what we decided about the validator field being required for every typeIO, it looks like you added assertions to phetioInherit, which I think is likely great and the way we should go. I also noticed that EnumerationIO could use its parent validator if it didn't specify its own (which is just its parent anyways). Should we inherit them? I'm leaning towards no, but wanted to bring it up just in case.

[zepumph]

Should we inherit them? I'm leaning towards no, but wanted to bring it up just in case.

Now that IO Types are es6 classes, inheritance is supported. That said we have ObjectIO.validateSubtype and it is working well. I also see that EnumerationIO has its own validator.

i still see one more TODO for this issue:

// x === phet.phetio.phetioCommandProcessor not exist yet, see https://github.com/phetsims/axon/issues/226
// TODO: Can this be eliminated? see https://github.com/phetsims/phet-io/issues/1371
PhetioCommandProcessorIO.validator = { isValidValue: x => true };
PhetioCommandProcessorIO.typeName = 'PhetioCommandProcessorIO';
ObjectIO.validateSubtype( PhetioCommandProcessorIO );

[zepumph]

Looks like no more TODOs now that we have IOTypes declared in the core types. Closing.