Delete StringEnumerationProperty

[zepumph]

From https://github.com/phetsims/chipper/issues/1106

[zepumph]

@samreid, does this feel right to you, once all usages have been changed?

[zepumph]

There are currently 9 usages of new StringEnumerationProperty. Presumably these will be refactored to use Enumeration.js

[samreid]

At today's dev meeting, we agreed to eliminate StringEnumerationProperty and use EnumerationValue instead.

[zepumph]

There are still 9 usages of StringEnuemerationProperty. I'll add deprecated markings to it.

[samreid]

I'll be doing some of this work in https://github.com/phetsims/circuit-construction-kit-common/issues/827

[samreid]

Deleted, closing.

[samreid]

We have discussed that the string literal enumerations are nice and it would be good to have support for them. From the notes:

TODO: The string literal union seems to have better autocomplete. And doesn't need an import.

I also recall @pixelzoom saying string literal unions worked better with Query String Machine.

I set up this poll on our TypeScript channel:

[pixelzoom]

This is an odd way to vote for an “OR” choice. Nothing preventing me from 👍 for both, so I’m going to do so. Imo StringUnionProperty is a superior name, more idiomatic, and distances us from attempts to prohibit union types in favor of EnumerationValue.

[samreid]

It is actually and/or since we can have both or neither. I changed the wording in the voting prompt.

[pixelzoom]

Changing my vote. I haven’t felt any need for QSM support of EnumerationValue. Very slight 👍for StringUnionProperty, if time permits. I haven’t felt inconvenienced by this pattern, not sure if it’s worth the effort to remove 2 (?) lines of bolierplate:

    readonly raysTypeProperty: Property<MyStringUnion>;

    this.myStringUnionProperty = new Property( 'someValue', {
      validValues: MyStringUnionValues,
      tandem: options.tandem.createTandem( 'myStringUnionProperty' ),
      phetioType: Property.PropertyIO( StringIO )
    } );

[pixelzoom]

Some additional thoughts about supporting EnumerationValue in QSM...

The 'custom' type is already supported by QSM, and I use it regularly to support types like Range, RangeWidthValue, Color,... Search for 'custom' in GOQueryParameters.ts for examples of RangeWidthValue. So it should be relatively easy to add support for a specific EnumerationValue type. And I wonder whether it's really necessay (or woth the effort) to add native support for EnumerationValue (if that was the intention). String unions are imo a superior solution here. I would consider EnumerationValue only if I had a rich enum that also needed to be settable via a query parameter.

Before doing anything related to EnumerationValue and QSM, there's a more fundamental problem: EnumerationValue and query parameters have incompatible string formats. EnumerationValues are declared as static constants, with uppercase names like DOT_PLOT and LINE_PLOT. And EnumerationValue (too cleverly imo) converts those to uppercase strings 'DOT_PLOT' and 'LINE_PLOT'. Those uppercase strings are unacceptable for use as query-parameter values, which uses lowerCamelCase. (We had this discussuon in Geometric Optics with the focalLengthControl query parameter, which is one reason why I'm using a string union instead of Enumeration.) So before putting effort into supporting EnumerationValue in QSM, you'll need to address this incompatibility. Automatically converting between uppercase and lowerCamelCase is imo not a path that should be taken. Relaxing the convention for naming static EnumerationValues would be a better approach.

[pixelzoom]

When @samreid announced this poll in Slack, he said:

context is https://github.com/phetsims/center-and-spread/blob/fa40acdfd780ea882baeeff82d3c9d6adeed9289/js/common/CASQueryParameters.ts

... which is:

  // TODO: It would be nice if QueryStringMachine supported mapping to EnumerationValue
  // TODO: Or should we bring back StringEnumerationProperty in https://github.com/phetsims/axon/issues/373?
  plotType: {
    type: 'string',
    validValues: [ 'dotPlot', 'linePlot' ],
    defaultValue: 'linePlot',
    public: true
  }

This looks like an appropriate place to use a string union. I would change to validValue: PlotTypeValues, and define the union like this:

// PlotType.ts
export const PlotTypeValues = [ 'dotPlot', 'linePlot' ] as const;
export type PlotType = ( typeof PlotTypeValues )[number];

Geometric Optics uses this string union pattern in 4 places: FocalLengthModelType.ts, OpticalImageType.ts, OpticShape.ts, and RaysType.ts. FocalLengthModelType.ts is used with query parameter focalLengthControl.

[samreid]

The results of the poll are:

@pixelzoom voted for StringUnionProperty, as described in https://github.com/phetsims/axon/issues/373#issuecomment-1059287885. There were no other votes.

@chrisklus and I tried using the custom feature of Query String machine and it worked great, thanks!

UPDATE: Oops, I forgot there was a stack overflow error during deep equals comparison of EnumerationValue types

[pixelzoom]

We're not deleting StringEnumertionProperty, and in fact have updated it for TypeScript. See https://github.com/phetsims/axon/issues/394. Closing.

[samreid]

As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.

[samreid]

I moved the sim-specific TODO to a sim-specific issue, closing.