unify IO type approach

[pixelzoom]

From review of IOObject in #36 (comment):

I said:

To clarify... It looks like you applied this mostly (solely?) to view types. View types (subtypes of scenery nodes) continue to use the old approach.

@samreid replied:

Yes, scenery currently has an incompatible implementation of PhET-iO support, but the APIs are being designed to match up. For instance, the IOObject options match the Node options.

I said:

Then why not use IOObject in scenery? Or factor out the options to be used in both IOObject and Node? The need to keep Node and IOObject options in-sync creates a longterm maintenance issue.

[samreid]

@jonathanolson can we change Node.js to extend IOObject.js instead of Object? Notes on mixin vs inheritance are here, my recommendation is to use inheritance if feasible: #31 (comment)

[samreid]

Initial investigation is promising, I committed changes to a new branch phetsims/scenery#721. This eliminates 188 lines of code from Node.js (a good trade to switch to 82 lines of code from IOObject.js) and more importantly unifies the phet-io instrumentation implementation. I tested a few instrumented nodes in Faraday's Law instance proxies and it seemed OK.

Ultimately @jonathanolson will need to review, but I'm wondering if @zepumph or @pixelzoom should take a look first?

[jonathanolson]

It has my approval (just reviewed the above commit), just fix the parentheses-space formatting that got reverted.

[pixelzoom]

Since @jonathanolson approved, I'm going to skip review.

[zepumph]

I don't see how it will work if tandem or options are passed through the Node.mutate call, how will that get up to IOObject? Are they not allowed to? If not then that will change convention/code in many places around the project.

[samreid]

Good point @zepumph, I'll look for some cases that exhibit that pattern and look for ideas.

[jonathanolson]

Ahh, I assumed that IOObject would have something like mutate that would do a similar operation. It definitely would be inconvenient if mutate couldn't be used for it.

[samreid]

@jonathanolson and I discussed and reasoned that something like:

Node.call( this, IOObject.getOptions( options ) );

May be suitable to (a) assign these non-mutable values once in IOObject and (b) keep mutate logic out of IOObject.

[zepumph]

That may be much harder to maintain than adding some sort of logic in Node.mutate that calls up to IOObject.

In terms of conventional efficiency, it adds a large correctional step to bridge the gap from phet to phet-io.

I found 180 files with this regex Node\.call\( this \)(.*\n)*.*\.mutate where a direct descendant of Node uses mutate instead of the parent call for options. Wouldn't it be easier for IOObject to support mutate than to touch all of those places (this doesn't even count Node subtypes).

Update: only 184 matches with \.call\( this \)(.*\n)*.*\.mutate

Update 2: I see 345 uses of Node that call with options directly.

[samreid]

It seems unfortunate that IOObject should support mutate since phetioState, tandem, phetioInstanceDocumentation, phetioEvents, phetioReadOnly, phetioType should not be mutable.

I know this unconventional (is crazy too strong a term?), but what about putting the IOObject.call(this,options) call in Node.mutate? If options are supplied to the Node constructor, then they will pass through in the mutate called from the Node constructor. If no options were supplied to the constructor, but then mutate call is called, then the options will be supported then. If no options were passed to Node() or mutate() then we will have a broken Node instance (it will lack the phetio attributes).

[samreid]

Or maybe it would be approximately the same to have a mutate call in IOObject, but how do we enforce that it gets called exactly once?

[zepumph]

Mutate can get called a lot, as it is passed up the chain, but maybe we could have a constructed flag in IOObject.

[samreid]

Mutate can get called a lot, as it is passed up the chain, but maybe we could have a constructed flag in IOObject.

I thought it would cause problems if it is called more than once in a hierarchy? Also agreed we could track a constructed flag in IOObject.

[pixelzoom]

There are 2 calls to initializePhetioObject:

// PhetioObject:
    if ( options ) {
      this.initializePhetioObject( {}, options );
    }

// Node
    mutate: function( options ) {

      if ( !options ) {
        return this;
      }

      this.initializePhetioObject( { phetioType: NodeIO }, options );

So initializePhetioObject is always called with options. The assertion I mentioned above states that, and the JSdoc confirms that it's not optional: @param {Object} options.

[pixelzoom]

Btw... I'm OK with allowing initializePhetioObject to be called without options, if you think there's some value in that. But if that the case, the doc should be changed to @param {Object} [options] and assert( options, ... ) should be deleted.

[samreid]

I believe that this assertion should move to the top of the constructor

Did you mean to move this assertion to the top of initializePhetioObject?

[pixelzoom]

Ah sorry, yes - I meant to move assert( options, ... ) to the top of initializePhetioObject, not the constructor.

[samreid]

OK that sounds good, I'll move it now.

[samreid]

I moved the assert statement, anything else for this issue?

[pixelzoom]

Nope, that's it, closing.