Review and Testing for IO Types

[samreid]

From #211, we have made significant improvements to IO Types, and more changes are planned in issues like:

#212
https://github.com/phetsims/phet-io/issues/1709
#213

We want to be confident these changes are all set for production. Here is the thread from #211:

All TODOs have been addressed or moved to other issues. The last topic for this issue is how it should be tested. Some options:

1. (lazy) We alert QA to these changes the next time they are dev or RC testing SHAs that contain these changes.
2. (eager) We create a dedicated version and request up-front testing before other SHAs from master are tested.

The lazy approach may be sufficient in this case, but we have discussed moving toward the eager approach for certain things. @zepumph and @ariel-phet what do you recommend?

[samreid]

@zepumph replied:

I think part of what leans me towards the lazy approach is the difficulty in pin pointing exactly what may have regressed. It may be easier to test eagerly in cases where you can give a runnable to QA and have them go at it. For this issue I would worry about (in no order):

• Type doc changing
• Methods from the wrapper no longer working
• A bug in IOType that causes an API change
• Something that may not have been converted correctly in a specific sim. Hopefully this would get caught on CT, but in the worst case a sim may have a silent state-related bug that.

Nothing above feels like something that QA can tackle efficiently. Likely CT took us 80% there and a code review can take us the rest. What do you think @samreid?

[samreid]

@ariel-phet said:

@samreid @zepumph perhaps a bit of both - considering phetsims/qa#519 is high on the testing list currently (EFAC will come first when the next RC is ready, but GAO is actively being tested) - perhaps after some review, GAO will be ready for its next dev test soon and can include these changes with QA alerted. That should help vet the new approach I would think.

[samreid]

Dev testing for Gravity and Orbits completed and no issues related to this were identified. Based on the preceding 2 comments, can @zepumph review and test the IO type changes (see bullet list 2 comments beforehand)? Perhaps this should block next PhET-iO RCs.

[zepumph]

Methods from the wrapper no longer working

I feel like studio is testament that this works well.

I looked at the rendered jsdoc, though I don't think that was the point of this review, and found a bug fixed in the above commit.

I noticed phetsims/scenery#1047 (comment).

• This default is buggy, and doesn't support complex IO Types. It should be rewritten to find the index of the first usage of IO and take a substring to that:

      // {string} Documentation that appears in PhET-iO Studio, supports HTML markup.
      documentation: `IO Type for ${ioTypeName.substring( 0, ioTypeName.length - PhetioConstants.IO_TYPE_SUFFIX.length )}`,

• I don't understand the value of wrapForPhetioCommandProcessor as an option/schema in the IO Type interface. I can't think of another type that will use this besides FunctionIO. I feel like we should either (a) get rid of it and hard code logic for functions in the command processor, or (b) rename and make it more explicit what its purpose is, and that it is only useful for functions.

• This chunk seems unused, half baked, and like it is part of https://github.com/phetsims/phet-io/issues/1657. Can we comment it out with a TODO linked to that issue?

    if ( this.hasOwnProperty( 'api' ) ) {
      assert && assert( this.api instanceof Object, 'Object expected for api' );
      assert && assert( Object.getPrototypeOf( this.api ) === Object.prototype, 'no extra prototype allowed on API object' );
    }

I promoted this to a TODO so that it would be dealt with here:
// TODO: What about EmitterIO<> that has no parameter types? See #217

@samreid please take a look at my suggestions and commit. Everything here seems pretty minor. I'd be fine with you taking off the blocking label, but I think we should try to finish up these loose ends eagerly.

[samreid]

Thanks for the feedback. I addressed them. I didn't see a robust way to detect FunctionIO in phetioCommandProcessor, so I gave the option a better name.

Should we move

// TODO: What about EmitterIO<> that has no parameter types? See #217

to another issue? Can you recommend how to process that case?

Please review my commits. Can this issue be closed?

[zepumph]

Yes new issue sounds good. Please create.

EDIT: I'll make one now, at 10:35MT 10/29

[zepumph]

Everything else looks great.

[zepumph]

I created #223, I'm going to close this issue.