Audit StateSchema.asValue.displayStrings for consistency

[samreid]

From https://github.com/phetsims/tandem/issues/303

      // TODO: This is the only place that has IO suffix in StateSchema.asValue, and likely change "<" to "(", https://github.com/phetsims/tandem/issues/303
      stateSchema: StateSchema.asValue( `StringUnionIO<${typeName}>`, {
          isValidValue: value => unionValues.includes( value )
        }
      )
    } ) );

[samreid]

@zepumph and I discussed that this may be prohibitively expensive to fix and maintenance release for hydrogen, but would be good to address as a "breaking change" for helium. We will discuss at upcoming phet-io meeting.

Should we make a helium label?

[samreid]

Proposed patch:

```diff Subject: [PATCH] Proposed changes for schema value names, see https://github.com/phetsims/tandem/issues/306 --- Index: js/types/EnumerationIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/EnumerationIO.ts b/js/types/EnumerationIO.ts --- a/js/types/EnumerationIO.ts (revision 8b679518c8f20b6ef06b4aa1febe5c6fc57e2736) +++ b/js/types/EnumerationIO.ts (date 1694558914342) @@ -14,7 +14,7 @@ // Cache each parameterized IOType so that it is only created once. const cache = new Map, IOType>(); -const joinKeys = ( keys: string[] ) => keys.join( '|' ); +const joinKeys = ( keys: string[] ) => keys.map( key => `'${key}'` ).join( '|' ); const EnumerationIO = ( enumerationContainer: EnumerationContainer ): IOType => { const enumeration = enumerationContainer.enumeration; Index: js/types/StringUnionIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/StringUnionIO.ts b/js/types/StringUnionIO.ts --- a/js/types/StringUnionIO.ts (revision 8b679518c8f20b6ef06b4aa1febe5c6fc57e2736) +++ b/js/types/StringUnionIO.ts (date 1694559076399) @@ -15,11 +15,13 @@ const StringUnionIO = ( unionValues: ParameterType ): IOType => { + const joinKeys = ( keys: ParameterType ) => keys.map( key => `'${key}'` ).join( '|' ); + assert && assert( unionValues, 'StringUnionIO needs unionValues' ); if ( !cache.has( unionValues ) ) { - const typeName = unionValues.join( ',' ); - cache.set( unionValues, new IOType( `StringUnionIO<${typeName}>`, { + const typeName = joinKeys( unionValues ); + cache.set( unionValues, new IOType( `StringUnionIO(${typeName})`, { documentation: 'An IOType validating on specific string values.', isValidValue: instance => unionValues.includes( instance ), @@ -27,8 +29,7 @@ toStateObject: _.identity, fromStateObject: _.identity, - // TODO: This is the only place that has IO suffix in StateSchema.asValue, see https://github.com/phetsims/tandem/issues/306 - stateSchema: StateSchema.asValue( `StringUnionIO<${typeName}>`, { + stateSchema: StateSchema.asValue( typeName, { isValidValue: value => unionValues.includes( value ) } ) Index: js/types/NullableIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/NullableIO.ts b/js/types/NullableIO.ts --- a/js/types/NullableIO.ts (revision 8b679518c8f20b6ef06b4aa1febe5c6fc57e2736) +++ b/js/types/NullableIO.ts (date 1694557053484) @@ -39,7 +39,7 @@ // If the argument is null, returns null. Otherwise, converts a state object to an instance of the underlying type. fromStateObject: stateObject => stateObject === null ? null : parameterType.fromStateObject( stateObject ), - stateSchema: StateSchema.asValue( `null|<${parameterType.typeName}>`, { + stateSchema: StateSchema.asValue( `null|${parameterType.typeName}`, { isValidValue: value => value === null || parameterType.isStateObjectValid( value ) } ) Index: js/types/DynamicMarkerIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/DynamicMarkerIO.ts b/js/types/DynamicMarkerIO.ts --- a/js/types/DynamicMarkerIO.ts (revision 5defe06c14365dac483b1ee5bfb67149ac8a935f) +++ b/js/types/DynamicMarkerIO.ts (date 1696522485783) @@ -10,6 +10,7 @@ import tandemNamespace from '../tandemNamespace.js'; import IOType from './IOType.js'; import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js'; +import StateSchema from './StateSchema.js'; const DynamicMarkerIO = new IOType( 'DynamicMarkerIO', { supertype: IOType.ObjectIO, @@ -17,6 +18,9 @@ toStateObject: () => { return {}; // empty object just as a placeholder }, + stateSchema: StateSchema.asValue( 'DynamicMarker', { + isValidValue: value => value instanceof Object && typeof value === 'object' && Object.keys( value ).length === 0 + } ), isValidValue: _.stubTrue, // accepts any type. documentation: 'IO Type used as a place holder for dynamic elements to be created when set for state.' } ); ```

[samreid]

See also https://github.com/phetsims/phet-io/issues/1954

[zepumph]

I renamed the issue since we went through all usages of StateSchema.asValue.

[zepumph]

We made a project board to capture this data and schedule changes for this.

[zepumph]

• [ ] OrIO should remove its space from joining its parameter state values.
• [ ] Do the items in the patch in https://github.com/phetsims/tandem/issues/306#issuecomment-1716625334

[zepumph]

• [ ] DynamicMarkerIO needs a stateSchema entry. Discovered in https://github.com/phetsims/phet-io-wrappers/issues/568
• [ ] Create some logic in IOType that ensures that stateSchemas MUST be provided if toStateObjects are.

[zepumph]

Create some logic in IOType that ensures that stateSchemas MUST be provided if toStateObjects are.

Let's do that now.

[zepumph]

I added this patch and immediately ran into trouble. We have toStateObjects for VoidIO, ObjectIO, and likely others, but no state schema. That doesn't seem right. Let's work on that!

``` Subject: [PATCH] fdsa --- Index: js/types/IOType.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/IOType.ts b/js/types/IOType.ts --- a/js/types/IOType.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/js/types/IOType.ts (date 1704308415101) @@ -257,6 +257,8 @@ assert && assert( !this.stateSchema || ( toStateObjectSupplied || this.stateSchema.isComposite() ), 'toStateObject method must be provided for value StateSchemas' ); + assert && toStateObjectSupplied && assert( stateSchemaSupplied || typeName === 'ObjectIO', 'state schemas are required if providing a toStateObject' ); + this.toStateObject = ( coreObject: T ) => { validate( coreObject, this.validator, VALIDATE_OPTIONS_FALSE );

[zepumph]

Ok. I don't think I can get to a commit point here. I found 3 spots where we weren't supplying stateSchema but should have been. I added this into the patch from above, so that the below is just about all work we want to do for this issue. Likely some of these API changes will never be worth it. We will just have to see.

```diff Subject: [PATCH] fdsa --- Index: tandem/js/types/DynamicMarkerIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/DynamicMarkerIO.ts b/tandem/js/types/DynamicMarkerIO.ts --- a/tandem/js/types/DynamicMarkerIO.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/DynamicMarkerIO.ts (date 1704315561033) @@ -10,6 +10,7 @@ import tandemNamespace from '../tandemNamespace.js'; import IOType from './IOType.js'; import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js'; +import StateSchema from './StateSchema.js'; const DynamicMarkerIO = new IOType( 'DynamicMarkerIO', { supertype: IOType.ObjectIO, @@ -17,6 +18,9 @@ toStateObject: () => { return {}; // empty object just as a placeholder }, + stateSchema: StateSchema.asValue( 'DynamicMarker', { + isValidValue: value => value instanceof Object && typeof value === 'object' && Object.keys( value ).length === 0 + } ), isValidValue: _.stubTrue, // accepts any type. documentation: 'PhET-iO Type used as a place holder for dynamic elements to be created when set for state.' } ); Index: tandem/js/types/VoidIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/VoidIO.ts b/tandem/js/types/VoidIO.ts --- a/tandem/js/types/VoidIO.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/VoidIO.ts (date 1704315273056) @@ -9,15 +9,17 @@ import tandemNamespace from '../tandemNamespace.js'; import IOType from './IOType.js'; +import StateSchema from './StateSchema.js'; /** * We sometimes use VoidIO as a workaround to indicate that an argument is passed in the simulation side, but * that it shouldn't be leaked to the PhET-iO client. */ -const VoidIO = new IOType( 'VoidIO', { +const VoidIO = new IOType( 'VoidIO', { isValidValue: () => true, documentation: 'Type for which there is no instance, usually to mark functions without a return value', - toStateObject: () => undefined + toStateObject: () => undefined, + stateSchema: StateSchema.asValue( 'undefined', { valueType: undefined } ) } ); tandemNamespace.register( 'VoidIO', VoidIO ); Index: tandem/js/types/EnumerationIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/EnumerationIO.ts b/tandem/js/types/EnumerationIO.ts --- a/tandem/js/types/EnumerationIO.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/EnumerationIO.ts (date 1704315561033) @@ -15,7 +15,7 @@ // Cache each parameterized IOType so that it is only created once. const cache = new IOTypeCache>(); -const joinKeys = ( keys: string[] ) => keys.join( '|' ); +const joinKeys = ( keys: string[] ) => keys.map( key => `'${key}'` ).join( '|' ); const EnumerationIO = ( enumerationContainer: EnumerationContainer ): IOType => { const enumeration = enumerationContainer.enumeration; Index: tandem/js/types/IOType.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/IOType.ts b/tandem/js/types/IOType.ts --- a/tandem/js/types/IOType.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/IOType.ts (date 1704315621791) @@ -257,6 +257,11 @@ assert && assert( !this.stateSchema || ( toStateObjectSupplied || this.stateSchema.isComposite() ), 'toStateObject method must be provided for value StateSchemas' ); + assert && toStateObjectSupplied && assert( stateSchemaSupplied || typeName === 'ObjectIO' || + + // TODO: What about an ancestor, not a direct supertype? https://github.com/phetsims/tandem/issues/306 + this.supertype?.stateSchema, 'state schemas are required if providing a toStateObject' ); + this.toStateObject = ( coreObject: T ) => { validate( coreObject, this.validator, VALIDATE_OPTIONS_FALSE ); Index: utterance-queue/js/SpeechSynthesisAnnouncer.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/SpeechSynthesisAnnouncer.ts b/utterance-queue/js/SpeechSynthesisAnnouncer.ts --- a/utterance-queue/js/SpeechSynthesisAnnouncer.ts (revision 559b856d9e578e8c266c16c5e967fe8a9f07e0eb) +++ b/utterance-queue/js/SpeechSynthesisAnnouncer.ts (date 1704315353735) @@ -42,6 +42,7 @@ import validate from '../../axon/js/validate.js'; import Validation from '../../axon/js/Validation.js'; import { Locale } from '../../joist/js/i18n/localeProperty.js'; +import StringIO from '../../tandem/js/types/StringIO.js'; // If a polyfill for SpeechSynthesis is requested, try to initialize it here before SpeechSynthesis usages. For // now this is a PhET specific feature, available by query parameter in initialize-globals. QueryStringMachine @@ -897,6 +898,7 @@ const SpeechSynthesisVoiceIO = new IOType( 'SpeechSynthesisVoiceIO', { isValidValue: v => true, // SpeechSynthesisVoice is not available on window + supertype: StringIO, toStateObject: speechSynthesisVoice => speechSynthesisVoice.name } ); Index: tandem/js/types/StringUnionIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/StringUnionIO.ts b/tandem/js/types/StringUnionIO.ts --- a/tandem/js/types/StringUnionIO.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/StringUnionIO.ts (date 1704315561053) @@ -16,11 +16,13 @@ const StringUnionIO = ( unionValues: ParameterType ): IOType => { + const joinKeys = ( keys: ParameterType ) => keys.map( key => `'${key}'` ).join( '|' ); + assert && assert( unionValues, 'StringUnionIO needs unionValues' ); if ( !cache.has( unionValues ) ) { - const typeName = unionValues.join( ',' ); - cache.set( unionValues, new IOType( `StringUnionIO<${typeName}>`, { + const typeName = joinKeys( unionValues ); + cache.set( unionValues, new IOType( `StringUnionIO(${typeName})`, { documentation: 'A PhET-iO Type validating on specific string values.', isValidValue: instance => unionValues.includes( instance ), @@ -28,8 +30,7 @@ toStateObject: _.identity, fromStateObject: _.identity, - // TODO: This is the only place that has IO suffix in StateSchema.asValue, see https://github.com/phetsims/tandem/issues/306 - stateSchema: StateSchema.asValue( `StringUnionIO<${typeName}>`, { + stateSchema: StateSchema.asValue( typeName, { isValidValue: value => unionValues.includes( value ) } ) Index: tandem/js/types/NullableIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/types/NullableIO.ts b/tandem/js/types/NullableIO.ts --- a/tandem/js/types/NullableIO.ts (revision 53f89327aea1358371bdbc57efbaab3bfdf986f3) +++ b/tandem/js/types/NullableIO.ts (date 1704315561049) @@ -40,7 +40,7 @@ // If the argument is null, returns null. Otherwise, converts a state object to an instance of the underlying type. fromStateObject: stateObject => stateObject === null ? null : parameterType.fromStateObject( stateObject ), - stateSchema: StateSchema.asValue( `null|<${parameterType.typeName}>`, { + stateSchema: StateSchema.asValue( `null|${parameterType.typeName}`, { isValidValue: value => value === null || parameterType.isStateObjectValid( value ) } )