Bring back support for missing `phetioValueType`

[pixelzoom]

At one time, PhET-iO would complain loudy when you forgot to provide phetioValueType. Now it silently defaults to Object, and display null for the inital and current values in Studio. That's just plain wrong, and is putting yet-another PhET-iO burden on the developer. Please bring back the developer support for this -- it's been biting me frequently.

Here's an example, from the most recent time that this has bitten me in https://github.com/phetsims/my-solar-system/issues/261#issuecomment-1776116935.

phetioValueType should be NumberIO here:

    this.speedProperty = new DerivedProperty( [ this.velocityProperty ], velocity => velocity.magnitude, {
      units: 'km/s',
      tandem: tandem.createTandem( 'speedProperty' ),
      phetioFeatured: true,
      phetioDocumentation: 'The magnitude of velocity'
    } );

And this is what Studio displays:

[samreid]

Is this a duplicate or related to https://github.com/phetsims/studio/issues/286?

[samreid]

Would it work to change ReadOnlyProperty from:

      if ( Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() && this.phetioState && Tandem.VALIDATION ) {
        assert && assert( options.phetioValueType !== IOType.ObjectIO, 'Stateful PhET-iO Properties must specify a phetioValueType: ' + this.phetioID );
      }

to avoid the phetioState guard? And think of this as a runtime assertion rather than type checking issue?

[zepumph]

Yes, I believe we should remove the phetioState guard. Good call! All Properties should have an actually helpful phetioValueType, and the default should just be for uninstrumented and/or phet brand cases.

[pixelzoom]

Is this a duplicate or related to https://github.com/phetsims/studio/issues/286?

Yes, sorry -- I forgot that I had reported essentially this same problem 11 months ago. This GitHub issue feels more general that https://github.com/phetsims/studio/issues/286 -- it's not a problem that's specific to Studio. Do as you wish to consolidate issues.

[samreid]

I made the change above locally and am fuzzing locally to see how it goes.

UPDATE: Local QA is green so far, so I'll commit the change.

[zepumph]

I believe that @pixelzoom ran into this problem because the assertion exists, but only when validating for phetio. MSS opts out of this check:

https://github.com/phetsims/my-solar-system/blob/fc93008bf5d32f0e080f1f0831caee1a88357055/package.json#L33-L35

[samreid]

OK I committed. @pixelzoom or @zepumph is there more to do here? Want to close/update/change https://github.com/phetsims/studio/issues/286 at all?

[pixelzoom]

I am fully pulled and have no working copy changes. With the speedProperty example shown in https://github.com/phetsims/axon/issues/443#issue-1959726256:

   this.speedProperty = new DerivedProperty( [ this.velocityProperty ], velocity => velocity.magnitude, {
      units: 'km/s',
      tandem: tandem.createTandem( 'speedProperty' ),
      // oops -- forgot phetioValueType: NumberIO
      phetioFeatured: true,
      phetioDocumentation: 'The magnitude of velocity'
    } );

.... I'm still seeing no errors when omitting phetioValueType. And Studio still shows DerivedPropertyIO<ObjectIO> and value null km/s.

[pixelzoom]

I thought this might be a problem with DerivedProperty, so I also tried with a Property. I added this to solar-system-common Body.ts:

    const someProperty = new Property( Vector2.ZERO, {
      tandem: tandem.createTandem( 'someProperty' )
      // oops -- forgot phetioValueType: Vector2.Vector2IO
    } );

Again, no errors, and Studio shows PropertyIO<ObjectIO> and value null.

[samreid]

Is this related to the message above? https://github.com/phetsims/axon/issues/443#issuecomment-1791003263

[pixelzoom]

Got it, validation: false in package.json was indeed why the missing phetioValueType was not failing. I don't agree that this is an API validation error -- it's a programming error, and should be noticed immediately, not sometime in the future when the validation flag becomes enabled.

[pixelzoom]

I think it should also be an error in phet brand, not just in phet-io brand. PhET-iO programming errors should be noted asap.

[zepumph]

Tandem.VALIDATION is not specific to API needs. It is about allowing a phet-io sim to run while you are just starting to work on it. I believe that the first things you should do in a PhET-iO retrofit is to get to a point where validation is turned on. This means passing in required tandems, correctly instrumenting Properties, and doing all the other items that the 40 usages of Tandem.VALIDATION guard against. Then you will be able to use these features. I am saying this sentence because over the years there have been many github issues trying to hone in on the scope of phet-io-specific instrumentation problems and assertions and when to fire them. We want to make sure that it is not prohibitively challenging to get a phet-io sim to a runnable state.

Perhaps a sync conversation is in order?

Also I want to note that @pixelzoom is a super-user instrumentation specialist at this point, and I don't want to downplay the value of being able to toggle a sim to phet-io brand supported, turn off validation, and see a sim in studio very very early in the process to better understand how PhET-iO works. Happy to discuss more.

[pixelzoom]

Yes, let's have a synchronous conversation.

[zepumph]

From discussion with @pixelzoom and @samreid, we decided that we agree with the following:

• phetioValidation:false is just for retrofits to get it to a starting point. Never use this for new sims.
• In general, phet-io problems are best reported in PhET-iO brand, because there is a lot of complexity in deciding how to tackle these kinds of problems, and when it is best to know about them. We also recognize that writing a phet brand sim to production first and then adding in PhET-iO will inherently reveal more challenging and confusing problems than if you developed PhET-iO in parallel.
• There are some Tandem.VALIDATION usages that are oversteps, and instead we can have logic more like "if you provided one phet-io option, then the other phet-io option better be right". Which can work in PhET brand too. Mostly the phetioValueType assertion for instrumented ReadOnlyProperties.
• New sim development for sims that support phet AND phet-io brand should be done in phet-io brand (as a superset of phet brand) to avoid simple mistakes that are only caught by Tandem.VALIDATION.

Masses and Springs and Normal Modes were the only spots that had instrumented Properties that weren't providing the right phetioValueType. Good work team. That is an improvement. @pixelzoom, I trust you have the right context about the trade-offs here, and so with that said please do let us know if you find another spot where you think that Tandem.VALIDATION shouldn't be guarding a phet-io assertion. Otherwise we are likely ready to close.

Oh! @pixelzoom, we discussed documenting the takeaways above, but to me they don't belong in the technical guide, and the current process checklist is deprecated. What did you have in mind?

[pixelzoom]

... @pixelzoom, we discussed documenting the takeaways above, but to me they don't belong in the technical guide, and the current process checklist is deprecated. What did you have in mind?

phet-io-instrumentation-process-checklist.md hasn't been deprecated yet. So for now, I made this change in phet-io-instrumentation-process-checklist.md, to discourage use of phet.phet-io.validation: false in package.json for new simulation development:

Old item:

• [ ] You can specify the ?phetioValidation=false query parameter or specify the package.json flag phet.phet-io.validation: false to opt out of errors during initial instrumentation. Once the simulation has attained a basic level of instrumentation, validation can be turned on to discover remaining issues. Remember to run grunt update after changing package.json.

Revised item:

• [ ] You can specify the ?phetioValidation=false query parameter or specify the package.json flag phet.phet-io.validation: false to opt out of errors during initial instrumentation. Do not use phet.phet-io.validation: false in package.json for new simulation development; it's intended for use in retrofitting existing sims with PhET-iO. Once the simulation has attained a basic level of instrumentation, validation can be turned on to discover remaining issues. Remember to run grunt update after changing package.json.

@zepumph @samreid if you disagree, feel free to reopen. Otherwise, closing.