Use StringEnumerationProperty for TypeScript string unions

[pixelzoom]

For https://github.com/phetsims/geometric-optics/issues/402, @jonathanolson added REVIEW comments to the cases where I'm creating a Property whose values are members of a string union.

For example, in GOOptions.ts:

focalLengthModelTypeProperty:
  //REVIEW: Consider StringEnumerationProperty?
    new Property<FocalLengthModelType>( GOQueryParameters.focalLengthControl, {
      validValues: FocalLengthModelTypeValues,
      tandem: optionsTandem.createTandem( 'focalLengthModelTypeProperty' ),
      phetioType: Property.PropertyIO( StringIO ),
...
    } )

I did not use StringEnumerationProperty because it was annotated as @deprecated by @samreid.

@samreid why is StringEnumerationProperty deprecated? It certainly looks like it's approprite to use it for TypeScript string unions.

[pixelzoom]

The 4 Properties that this affects are identified with TODOs in the above commit.

[samreid]

During Thursday's TypeScript meeting, we decided to approve/resurrect StringEnumerationProperty. Thanks for catching the deprecation warnings, I'll remove them.

[samreid]

OK committed. Can you please review the commit, which also updates the optionize call?

[pixelzoom]

Two things:

(1) StringEnumerationProperty should allow validValues to be optionally provided.

This is problematic:

type StringEnumerationPropertyOptions<T> = Omit<PropertyOptions<T>, 'validValues' | 'phetioType'>;

EnumerationProperty allows validValues to be specified because it's possible that only a subset of an enumeration may be valid. StringEnumerationProperty should allow validValues for the same reason. Geometric Optics has an example like this, see this.opticShapeProperty in Optic.ts. The string union is OpticShape.ts. For a lens, 'concave' and 'convex' shapes are supported. For a mirror, 'concave', 'convex' and 'flat' are supported.

(2) Since this is a Property whose values are from a string union, should StringEnumerationProperty be renamed to StringUnionProperty? It would be more aligned with TypeScript terminology. And it's doubtful that this Property subclass should be used in JavaScript code.

[samreid]

(1) StringEnumerationProperty should allow validValues to be optionally provided.

That is currently the required first argument. The signature is constructor( values: readonly T[], value: T, providedOptions?: StringEnumerationPropertyOptions<T> ) {. Does that seem OK? Or should we move it back to validValues and make it a required option?

(2) Since this is a Property whose values are from a string union, should StringEnumerationProperty be renamed to StringUnionProperty? It would be more aligned with TypeScript terminology. And it's doubtful that this Property subclass should be used in JavaScript code.

To me, it behaves like an enumeration so that seems an appropriate name.

Some notes-to-self for when I look at this later:

• Change to class StringEnumerationProperty<T extends string> so we cannot make StringEnumerationProperty<number>
• I observed that const m = new StringEnumerationProperty(['x','y']) infers the type correctly and gets the type parameter right. It may help us get rid of the as const and typeof a[number] pattern. UPDATE:

I found that

const x = new StringEnumerationProperty( [ 'a', 'b', 'c' ], 'a' );
type extractGeneric<Type> = Type extends StringEnumerationProperty<infer X> ? X : never;
type extracted = extractGeneric<typeof x>;

gets the type correct. But it's unclear how this could be used in practice. For instance, in GOModel.ts we need to annotate the type of raysTypeProperty in the class attribute declaration, and cannot refer to the type from the instantiated StringEnumerationProperty at that point.

[samreid]

I'm inclined to request votes for (1) and (2) above at dev meeting.

[pixelzoom]

5/5/2022 dev meeting: We spent considerable time discussing this (but I don't see any notes?) and decided to break into a subgroup. I think @samreid and @jonathanolson expressed interest, so assigning them.

[pixelzoom]

Using the current implementation of StringEnumerationProperty, here's what I end up with in GOModel.ts (not committed). It seems a little odd to me that the string union type (RaysType) is mentioned in the declaration, but not used in the instantiation. But maybe that's OK.

  public readonly raysTypeProperty: Property<RaysType>;
...
    this.raysTypeProperty = new StringEnumerationProperty( RaysTypeValues, 'marginal', {
      tandem: options.tandem.createTandem( 'raysTypeProperty' )
    } );

[pixelzoom]

In the above commit, geometric-optics is now using StringEnumerationProperty. This will provide examples for adjusting the API/implementation of StringEnumerationProperty.

[pixelzoom]

Since this is a general issue, transferring from geometric-optics to axon.

[samreid]

If I recall correctly, we agreed to move the required parameter into a required validValues option. I think @jonathanolson volunteered to make the change and update occurrences.

[pixelzoom]

Maybe I misunderstood. But did we have concerns about type-checking if we move the first param to validValues option? And the inconvenience of having to use as const for validValues?

[pixelzoom]

In Optic.ts, opticSurfaceTypeProperty is an example of a StringEnumerationProperty that uses a subset of values from OpticSurfaceType. I experimented with replacing StringEnumerationProperty's first parameter (values) with a required validValues option, and discovered that as const was not needed.

[samreid]

Here's a patch that makes validValues required:

```diff Index: main/axon/js/StringEnumerationProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/StringEnumerationProperty.ts b/main/axon/js/StringEnumerationProperty.ts --- a/main/axon/js/StringEnumerationProperty.ts (revision 13baf1e5dd482848b7c28854a3992fe51706ef01) +++ b/main/axon/js/StringEnumerationProperty.ts (date 1652303061504) @@ -2,8 +2,9 @@ import Property, { PropertyOptions } from './Property.js'; import StringIO from '../../tandem/js/types/StringIO.js'; import optionize from '../../phet-core/js/optionize.js'; +import PickRequired from '../../phet-core/js/types/PickRequired.js'; -type StringEnumerationPropertyOptions = Omit, 'validValues' | 'phetioType'>; +type StringEnumerationPropertyOptions = Omit, 'phetioType'> & PickRequired, 'validValues'>; /** * In TypeScript, it is common to use a string literal union as an enumeration. This type automatically specifies @@ -12,10 +13,9 @@ * @author Sam Reid (PhET Interactive Simulations) */ class StringEnumerationProperty extends Property { - constructor( values: readonly T[], value: T, providedOptions?: StringEnumerationPropertyOptions ) { + constructor( value: T, providedOptions?: StringEnumerationPropertyOptions ) { const options = optionize, {}, PropertyOptions>()( { - validValues: values, phetioType: Property.PropertyIO( StringIO ) }, providedOptions ); Index: main/geometric-optics/js/common/model/GOModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/model/GOModel.ts b/main/geometric-optics/js/common/model/GOModel.ts --- a/main/geometric-optics/js/common/model/GOModel.ts (revision ea598c5979fb7db18de4c469c68015d675819d69) +++ b/main/geometric-optics/js/common/model/GOModel.ts (date 1652303211224) @@ -94,7 +94,8 @@ this.optic = optic; - this.raysTypeProperty = new StringEnumerationProperty( RaysTypeValues, 'marginal', { + this.raysTypeProperty = new StringEnumerationProperty( 'marginal', { + validValues: RaysTypeValues, tandem: options.tandem.createTandem( 'raysTypeProperty' ) } ); Index: main/geometric-optics/js/common/view/GOScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/view/GOScreenView.ts b/main/geometric-optics/js/common/view/GOScreenView.ts --- a/main/geometric-optics/js/common/view/GOScreenView.ts (revision ea598c5979fb7db18de4c469c68015d675819d69) +++ b/main/geometric-optics/js/common/view/GOScreenView.ts (date 1652303246737) @@ -159,7 +159,8 @@ return new Bounds2( modelVisibleBounds.minX, -y, modelVisibleBounds.maxX, y ); } ); - const objectDragModeProperty = new StringEnumerationProperty( ObjectDragModeValues, options.objectDragMode, { + const objectDragModeProperty = new StringEnumerationProperty( options.objectDragMode, { + validValues: ObjectDragModeValues, tandem: providedOptions.tandem.createTandem( 'objectDragModeProperty' ), phetioReadOnly: true, phetioDocumentation: 'Controls dragging of the optical objects. ' + Index: main/geometric-optics/js/common/GOOptions.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/GOOptions.ts b/main/geometric-optics/js/common/GOOptions.ts --- a/main/geometric-optics/js/common/GOOptions.ts (revision ea598c5979fb7db18de4c469c68015d675819d69) +++ b/main/geometric-optics/js/common/GOOptions.ts (date 1652303136834) @@ -29,7 +29,8 @@ } ), focalLengthModelTypeProperty: - new StringEnumerationProperty( FocalLengthModelTypeValues, GOQueryParameters.focalLengthControl, { + new StringEnumerationProperty( GOQueryParameters.focalLengthControl, { + validValues: FocalLengthModelTypeValues, tandem: optionsTandem.createTandem( 'focalLengthModelTypeProperty' ), phetioDocumentation: 'Determines how focal length is modeled and controlled in the Lens and Mirror screens.
' + 'This can also be set via the focalLengthControl query parameter.
' + Index: main/geometric-optics/js/common/model/Optic.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/model/Optic.ts b/main/geometric-optics/js/common/model/Optic.ts --- a/main/geometric-optics/js/common/model/Optic.ts (revision ea598c5979fb7db18de4c469c68015d675819d69) +++ b/main/geometric-optics/js/common/model/Optic.ts (date 1652303156321) @@ -125,11 +125,11 @@ this.sign = options.sign; - this.opticSurfaceTypeProperty = new StringEnumerationProperty( - options.opticSurfaceTypes, options.opticSurfaceTypes[ 0 ], { - tandem: options.tandem.createTandem( 'opticSurfaceTypeProperty' ), - phetioDocumentation: 'surface type of the optic' - } ); + this.opticSurfaceTypeProperty = new StringEnumerationProperty( options.opticSurfaceTypes[ 0 ], { + validValues: options.opticSurfaceTypes, + tandem: options.tandem.createTandem( 'opticSurfaceTypeProperty' ), + phetioDocumentation: 'surface type of the optic' + } ); // In https://github.com/phetsims/geometric-optics/issues/262, it was decided that the optic should have a fixed // position, at the origin of the model coordinate frame. This differs from the Flash version, where the optic ```

It seems to be superior to having a required first arg. Want to commit it?

But did we have concerns about type-checking if we move the first param to validValues option?

Type inference seems to work when specifying options.validValues instead of just values.

Anything else for this issue?

[pixelzoom]

I applied the patch, looks good in geometric-optics.

I noticed a note in CAVQueryParameters.ts, and changed the TODO to point to this issue. Back to @samreid to decide whether to address it.

[samreid]

Thanks, I removed the comment. It seems this issue is ready to close. Anyone please reopen if there's more to do.