Eliminate the need for PhET-iO wrapper instances

samreid commented 4 years ago

From https://github.com/phetsims/tandem/issues/188#issuecomment-689779651

PhetioObject calls:

this.phetioWrapper = new this.phetioType( this, this.tandem.phetioID );

However, it is unimportant that the IO Type constructor be used to create the wrapper. The wrapper just consists of a phetioID and phetioObject. We should eliminate this unnecessary use of ObjectIO's constructor.

samreid commented 4 years ago

phetioCommandProcessor invokes the method like so:

const result = methodSignature.implementation.apply( wrapper, args );

I asked @zepumph which is better between:

    setValue: {
      returnType: VoidIO,
      parameterTypes: [ parameterType ],
      implementation: function( value ) {
        this.set( value );
      },
      documentation: 'Sets the value of the Property. If the value differs from the previous value, listeners are ' +
                     'notified with the new value.',
      invocableForReadOnlyElements: false
    },

and

    setValue: {
      returnType: VoidIO,
      parameterTypes: [ parameterType ],
      implementation: function( property,value ) {
        property.set( value );
      },
      documentation: 'Sets the value of the Property. If the value differs from the previous value, listeners are ' +
                     'notified with the new value.',
      invocableForReadOnlyElements: false
    },

And he recommended using this.

I mentioned we could create a backward compatible patch like so:

const result = methodSignature.implementation.apply( { phetioObject: targetObject }, args );

But using this may be clearest. I'll make that change as part of this changeset.

samreid commented 4 years ago

I have many things working nicely, and the wrapper type eliminated completely.

```diff Index: main/axon/js/ObservableArrayIO.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/axon/js/ObservableArrayIO.js (revision fc83bd549a1b358061c8393b0409d8c7bde48bf8) +++ main/axon/js/ObservableArrayIO.js (date 1599704243595) @@ -96,7 +96,7 @@ returnType: VoidIO, parameterTypes: [ FunctionIO( VoidIO, [ parameterType ] ) ], implementation: function( listener ) { - this.phetioObject.addItemAddedListener( listener ); + this.addItemAddedListener( listener ); }, documentation: 'Add a listener that is called when an item is added to the observable array.' }, @@ -110,7 +110,7 @@ returnType: VoidIO, parameterTypes: [ FunctionIO( VoidIO, [ parameterType ] ) ], implementation: function( listener ) { - this.phetioObject.addItemRemovedListener( listener ); + this.addItemRemovedListener( listener ); }, documentation: 'Add a listener that is called when an item is removed from the observable array.' }, @@ -122,7 +122,7 @@ returnType: NumberIO, parameterTypes: [], implementation: function() { - return this.phetioObject.length; + return this.length; }, documentation: 'Get the number of elements in the observable array' } Index: main/scenery/js/nodes/IndexedNodeIO.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/scenery/js/nodes/IndexedNodeIO.js (revision c6b0dc1e26c0cd533f027ea725723c8b19709c60) +++ main/scenery/js/nodes/IndexedNodeIO.js (date 1599704243638) @@ -36,7 +36,7 @@ returnType: VoidIO, parameterTypes: [], implementation: function() { - return this.phetioObject.moveForward(); + return this.moveForward(); }, documentation: 'Move this node one index forward in each of its parents. If the node is already at the front, this is a no-op.' }, @@ -45,7 +45,7 @@ returnType: VoidIO, parameterTypes: [], implementation: function() { - return this.phetioObject.moveBackward(); + return this.moveBackward(); }, documentation: 'Move this node one index backward in each of its parents. If the node is already at the back, this is a no-op.' } Index: main/charges-and-fields/js/charges-and-fields/model/ChargedParticle.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/charges-and-fields/js/charges-and-fields/model/ChargedParticle.js (revision 56a9c2215d7ed272069b44f605d8293e98105257) +++ main/charges-and-fields/js/charges-and-fields/model/ChargedParticle.js (date 1599704243621) @@ -75,7 +75,7 @@ returnType: VoidIO, parameterTypes: [ NumberIO ], implementation: function( value ) { - this.phetioObject.charge = value.charge; + this.charge = value.charge; }, documentation: 'Set charge (in units of e)', invocableForReadOnlyElements: false Index: main/axon/js/Emitter.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/axon/js/Emitter.js (revision fc83bd549a1b358061c8393b0409d8c7bde48bf8) +++ main/axon/js/Emitter.js (date 1599704243626) @@ -162,7 +162,7 @@ returnType: VoidIO, parameterTypes: [ FunctionIO( VoidIO, parameterTypes ) ], implementation: function( listener ) { - this.phetioObject.addListener( listener ); + this.addListener( listener ); }, documentation: 'Adds a listener which will be called when the emitter emits.' }, @@ -172,7 +172,7 @@ // Match `Emitter.emit`'s dynamic number of arguments implementation: function() { - this.phetioObject.emit.apply( this.phetioObject, arguments ); + this.emit.apply( this.phetioObject, arguments ); }, documentation: 'Emits a single event to all listeners.', invocableForReadOnlyElements: false Index: main/phet-io/js/PhetioEngineIO.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/phet-io/js/PhetioEngineIO.js (revision 371809713cf6b35ee3bb10c36663d95c8c19f1a0) +++ main/phet-io/js/PhetioEngineIO.js (date 1599697790220) @@ -21,12 +21,12 @@ import phetio from './phetio.js'; class PhetioEngineIO extends ObjectIO { - constructor( phetioEngine, phetioID ) { - super( phetioEngine, phetioID ); - - // @private - to simplify readability in the methods below - this.phetioEngine = this.phetioObject; - } + // constructor( phetioEngine, phetioID ) { + // super( phetioEngine, phetioID ); + // + // // @private - to simplify readability in the methods below + // this.phetioEngine = this.phetioObject; + // } } PhetioEngineIO.methods = { @@ -40,7 +40,7 @@ const values = {}; for ( let i = 0; i < phetioIDs.length; i++ ) { const phetioID = phetioIDs[ i ]; - const wrapper = this.phetioEngine.getWrapper( phetioID ); + const wrapper = this.phetioEngine.getPhetioObject( phetioID ); values[ phetioID ] = wrapper.getValue(); } return values; @@ -55,7 +55,7 @@ // 3rd is {Object} from PhetioStateEngine.getStateForObject() or {} if phetioState: false parameterTypes: [ FunctionIO( VoidIO, [ StringIO, ObjectIO, ObjectIO ] ) ], implementation: function( listener ) { - this.phetioEngine.addPhetioObjectAddedListener( listener ); + this.addPhetioObjectAddedListener( listener ); }, documentation: 'Adds a listener that receives a callback when a PhET-iO Element has been added.' + 'Arguments for the function: \n

phetioTypeName: The name of the PhET-iO Type\n

However, phet-io unit tests are failing because of the NodeIO properties.

samreid commented 4 years ago

On hold until https://github.com/phetsims/scenery/issues/1046 and possibly https://github.com/phetsims/scenery/issues/1047 are complete.

UPDATE FROM MK: and https://github.com/phetsims/scenery/issues/1082

samreid commented 4 years ago

One aspect of the patch was not blocked by the issues mentioned in the prior comment, so I decided to move forward with that change in order to (a) make that improvement and (b) prepare a more succinct patch since we may not return to this issue while it is fresh in our minds or before there is more churn in these files.

Here is the reduced patch after the above commits:

```diff Index: main/phet-io/js/phetioEngine.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/phet-io/js/phetioEngine.js (revision 46a3986178ad25b133afa31259b7fbe3182ec2b5) +++ main/phet-io/js/phetioEngine.js (date 1599731245340) @@ -373,23 +373,6 @@ return this.phetioObjectMap[ phetioID ].phetioType; } - /** - * Look up a component given a phetioID, or undefined if not registered - * @param {string} phetioID - * @returns {ObjectIO} - instantiated IO Type - * @public (phet-io-internal) - */ - getWrapper( phetioID ) { - - // Support calls on phetioEngine as part of phetioInitialized callback, even though it has not been registered yet - // through phetioObjectAdded as part of the Tandem batch registration process. - if ( phetioID === this.tandem.phetioID ) { - return this.phetioWrapper; - } - assert && this.assertPhetioObjectFound( phetioID ); - return this.phetioObjectMap[ phetioID ].phetioWrapper; - } - /** * For convenience * @param {string} phetioID @@ -397,6 +380,9 @@ * @public */ getPhetioObject( phetioID ) { + if ( phetioID === this.tandem.phetioID ) { // TODO: improve this + return this; + } assert && this.assertPhetioObjectFound( phetioID ); return this.phetioObjectMap[ phetioID ]; } @@ -417,7 +403,6 @@ * @private */ phetioObjectAdded( phetioObject ) { - assert && assert( phetioObject.phetioWrapper, 'Can only be registered after initialization' ); assert && assert( arguments.length === 1, 'Called with incorrect signature' ); assert && assert( phetioObject instanceof phet.tandem.PhetioObject, 'must be PhetioObject' ); assert && assert( phetioObject, 'Only non-falsy components should be registered' ); Index: main/phet-io/js/PhetioEngineIO.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/phet-io/js/PhetioEngineIO.js (revision 46a3986178ad25b133afa31259b7fbe3182ec2b5) +++ main/phet-io/js/PhetioEngineIO.js (date 1599731245335) @@ -33,7 +33,7 @@ const values = {}; for ( let i = 0; i < phetioIDs.length; i++ ) { const phetioID = phetioIDs[ i ]; - const wrapper = this.getWrapper( phetioID ); + const wrapper = this.phetioEngine.getPhetioObject( phetioID ); values[ phetioID ] = wrapper.getValue(); } return values; Index: main/phet-io/js/phetioCommandProcessor.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/phet-io/js/phetioCommandProcessor.js (revision 46a3986178ad25b133afa31259b7fbe3182ec2b5) +++ main/phet-io/js/phetioCommandProcessor.js (date 1599731127412) @@ -224,11 +224,13 @@ phet.phetio.phetioEngine.errorIfUndefined( phetioID ); // Load phetio from the namespace to avoid a module cycle - let wrapper = phet.phetio.phetioEngine.getWrapper( phetioID ); + let targetPhetioID = phetioID; + let targetObject = phet.phetio.phetioEngine.getPhetioObject( phetioID ); // If we are a LinkedElementIO, then forward the call onto the element that is being linked to. - if ( wrapper.phetioObject.phetioType === LinkedElementIO ) { - wrapper = phet.phetio.phetioEngine.getWrapper( wrapper.phetioObject.element.tandem.phetioID ); + if ( targetObject.phetioType === LinkedElementIO ) { + targetPhetioID = targetObject.element.tandem.phetioID; + targetObject = phet.phetio.phetioEngine.getPhetioObject( targetPhetioID ); } // Search up the chain to get the specified method. @@ -247,19 +249,19 @@ }; // {Object} - the method definition for an IO Type, see PropertyIO for examples - const methodSignature = getMethodSignature( wrapper.constructor, method ); + const methodSignature = getMethodSignature( targetObject.phetioType, method ); if ( !methodSignature ) { throw new Error( 'method not found: ' + phetioID + '.' + method ); } // Check for exact value of false instead of just truthiness because invocableForReadOnlyElements is often undefined - if ( wrapper.phetioObject.phetioReadOnly && methodSignature.invocableForReadOnlyElements === false ) { + if ( targetObject.phetioReadOnly && methodSignature.invocableForReadOnlyElements === false ) { throw new Error( 'Cannot invoke method \'' + method + '\' on read-only element \'' + phetioID + '\'' ); } // archetypes can't have methods called on them, see https://github.com/phetsims/phet-io-wrappers/issues/302 - if ( wrapper.phetioObject.phetioIsArchetype ) { + if ( targetObject.phetioIsArchetype ) { throw new Error( 'Cannot invoke method \'' + method + '\' on dynamic element archetype \'' + phetioID + '\'' ); } @@ -283,7 +285,7 @@ // Invoke the method. If it throws an error, it will be caught in handleSingleInvoke and returned to the client // See https://github.com/phetsims/phet-io/issues/941 - const result = methodSignature.implementation.apply( wrapper.phetioObject, args ); + const result = methodSignature.implementation.apply( targetObject, args ); // Serialize the result if possible. return methodSignature.returnType.toStateObject( result ); Index: main/tandem/js/PhetioObject.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- main/tandem/js/PhetioObject.js (revision 9422eb38230c82c00645ce24c98665b97831dbd0) +++ main/tandem/js/PhetioObject.js (date 1599731129112) @@ -151,11 +151,6 @@ // @public (read-only) {Object} - see docs at DEFAULTS declaration this.phetioComponentOptions = DEFAULTS.phetioComponentOptions; - // @public (read-only) {ObjectIO|null} - assigned in initializePhetioObject - the instantiated IO Type. The phetioWrapper - // is the API layer between the wrapper and the phetioObject. It's used to call methods on this phetioObject from - // the wrapper frame. Will be null if this PhetioObject is never instrumented. - this.phetioWrapper = null; - // @private {boolean} - track whether the object has been initialized. This is necessary because initialization // can happen in the constructor or in a subsequent call to initializePhetioObject (to support scenery Node) this.phetioObjectInitialized = false; @@ -336,9 +331,6 @@ if ( PHET_IO_ENABLED && this.isPhetioInstrumented() ) { assert && Tandem.VALIDATION && assert( !this.phetioType.uninstrumented, 'cannot instantiate a phetioType that should not be instrumented' ); - - // @public (read-only phet-io-internal) - this.phetioWrapper = new this.phetioType( this, this.tandem.phetioID ); } this.tandem.addPhetioObject( this ); this.phetioObjectInitialized = true; @@ -551,7 +543,6 @@ if ( this.phetioObjectInitialized ) { this.tandem.removePhetioObject( this ); - this.phetioWrapper && this.phetioWrapper.dispose && this.phetioWrapper.dispose(); } // Dispose LinkedElements ```

samreid commented 4 years ago

I implemented a pattern for this as part of https://github.com/phetsims/tandem/issues/211. @zepumph can you review this part in isolation? See IOType.createWrapper and its usages.

zepumph commented 4 years ago

This makes a lot of sense. Thanks for taking this on. I'll stay assigned until those issues blocking issues are dealt with.

samreid commented 4 years ago

The title of this issue has been completed, since IOType is not a constructor for wrappers. However, now we would like to eliminate the wrapper completely, so I'll retitle the issue.

samreid commented 4 years ago

Implemented in the commits, and I moved https://github.com/phetsims/phet-io/issues/1712 to a side issue that I may need help on. @zepumph can you please review?

zepumph commented 4 years ago

Looks really good. I cleaned up "wrapper" terminology because we no longer think of IO Types as "wrappers" to the core type, and it is over-loaded with the other side of the cross-frame communication (wrapper side).

Closing, please reopen if there is anything else for you.

phetsims / tandem

Eliminate the need for PhET-iO wrapper instances #210