Closed pixelzoom closed 1 year ago
I didn't have an easy way to test this, so I thought I'd just commit a first pass and see if it handled the validation that you wanted. Pass it the string array, and feel free to update it to fix bugs or improve on it.
I tried using StringUnionIO in CurvePoint.ts:
- pointType: StringUnionIO<PointTypeValues>,
+ pointType: StringUnionIO<PointTypeValues>,
... and it's resulting in this error:
TS2322: Type '(unionValues: PointTypeValues) => IOType<any, any>' is not assignable to type 'IOType<any, any>'.
I also tried pointType: StringUnionIO<PointType>
, with similar TS error.
I poked around in StringUnionIO, but I have no idea how to fix this. @zepumph thoughts?
I also tried:
pointType: StringUnionIO<PointType>( PointTypeValues )
The root cause of this problem seems to be related to the fact that the array of string values (e.g PointTypeValues
) is const
, while StringUnionIO is expecting a mutable array for parameter unionValues
.
StringUnionIO seems to have problems . Here's a patch of where I got to, not sure if it's all correct.
pointType: StringUnionIO<PointType>( PointTypeValues )
still looks correct to me for CurvePoint.ts. But I'm still seeing this TS error:
TS2345: Argument of type 'readonly ["smooth", "cusp", "discontinuous"]' is not assignable to parameter of type '("smooth" | "cusp" | "discontinuous")[]'. The type 'readonly ["smooth", "cusp", "discontinuous"]' is 'readonly' and cannot be assigned to the mutable type '("smooth" | "cusp" | "discontinuous")[]'.
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 80cb5b991ea329bcc1772939000f99bde8ddcb3a)
+++ b/js/types/StringUnionIO.ts (date 1670522995053)
@@ -13,13 +13,13 @@
// Cache each parameterized IOType so that it is only created once
const cache = new Map<string[], IOType>();
-const StringUnionIO = <ParameterType extends string[]>( unionValues: ParameterType ): IOType => {
+const StringUnionIO = <ParameterType extends string>( unionValues: ParameterType[] ): IOType<string, string> => {
assert && assert( unionValues, 'StringUnionIO needs unionValues' );
if ( !cache.has( unionValues ) ) {
const typeName = unionValues.join( ',' );
- cache.set( unionValues, new IOType<string, string>( `StringUnionIO<${typeName}>`, {
+ cache.set( unionValues, new IOType<ParameterType, string>( `StringUnionIO<${typeName}>`, {
documentation: 'An IOType validating on specific string values.',
isValidValue: instance => unionValues.includes( instance ),
If I do this:
pointType: StringUnionIO<PointType>( [ ...PointTypeValues ] ),
... then the TS error goes away. So this confirms that the problem is with the const
-ness of PointTypeValues
.
I added readonly
to StringUnionIO
and it seems to be working OK. I tested in CurvePoint like so:
Subject: [PATCH] test
---
Index: js/common/model/CurvePoint.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CurvePoint.ts b/js/common/model/CurvePoint.ts
--- a/js/common/model/CurvePoint.ts (revision 4e77c506fc435af2a484e390984caa1a87254a3a)
+++ b/js/common/model/CurvePoint.ts (date 1671058788346)
@@ -17,7 +17,7 @@
import Vector2 from '../../../../dot/js/Vector2.js';
import IOType from '../../../../tandem/js/types/IOType.js';
import NumberIO from '../../../../tandem/js/types/NumberIO.js';
-import StringIO from '../../../../tandem/js/types/StringIO.js';
+import StringUnionIO from '../../../../tandem/js/types/StringUnionIO.js';
import calculusGrapher from '../../calculusGrapher.js';
import CalculusGrapherConstants from '../CalculusGrapherConstants.js';
@@ -151,7 +151,7 @@
return {
x: this.x,
y: this.y,
- pointType: this.pointType,
+ pointType: this.pointType, // Note if you put 'hello' here it will get caught as a type error, nice!
initialY: this.initialState.y,
initialPointType: this.initialState.pointType
};
@@ -174,9 +174,9 @@
stateSchema: {
x: NumberIO,
y: NumberIO,
- pointType: StringIO,
+ pointType: StringUnionIO( PointTypeValues ),
initialY: NumberIO,
- initialPointType: StringIO
+ initialPointType: StringUnionIO( PointTypeValues )
},
toStateObject: ( curvePoint: CurvePoint ) => curvePoint.toStateObject(),
fromStateObject: ( stateObject: CurvePointStateObject ) => CurvePoint.fromStateObject( stateObject ),
And it correctly caught when trying to use an incorrect value.
@zepumph and/or @pixelzoom want to review?
Looks like you also added proper parameterization of IOType, nice. This is working great in calculus-grapher and geometric-options.
I think we should also change StringEnumerationProperty to phetioValueType: StringUnionIO
, see patch below. This will result in PhET-iO API changes. @samreid Do you agree?
Index: js/StringEnumerationProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/StringEnumerationProperty.ts b/js/StringEnumerationProperty.ts
--- a/js/StringEnumerationProperty.ts (revision 60f87e67c88ea61a74767d4eb60428cf47da3dab)
+++ b/js/StringEnumerationProperty.ts (date 1671064122886)
@@ -8,7 +8,7 @@
*/
import Property, { PropertyOptions } from './Property.js';
-import StringIO from '../../tandem/js/types/StringIO.js';
+import StringUnionIO from '../../tandem/js/types/StringUnionIO.js';
import optionize, { EmptySelfOptions } from '../../phet-core/js/optionize.js';
import PickRequired from '../../phet-core/js/types/PickRequired.js';
import StrictOmit from '../../phet-core/js/types/StrictOmit.js';
@@ -21,7 +21,7 @@
public constructor( value: T, providedOptions: StringEnumerationPropertyOptions<T> ) {
const options = optionize<StringEnumerationPropertyOptions<T>, EmptySelfOptions, PropertyOptions<T>>()( {
- phetioValueType: StringIO
+ phetioValueType: StringUnionIO( providedOptions.validValues )
}, providedOptions );
super( value, options );
Example in geometric-optics:
this.raysTypeProperty = new StringEnumerationProperty( 'marginal', {
validValues: RaysTypeValues,
tandem: options.tandem.createTandem( 'raysTypeProperty' )
} );
... I think we should also change StringEnumerationProperty to
phetioValueType: StringUnionIO
, see patch below. This will result in PhET-iO API changes.
I see 19 occurrences of new StringEnumerationProperty(
. Only 1 of those is instrunented and appears in a sim that has a PhET-iO API file: molecule-polarity.
EDIT: NumberPicker also has 2 instances of new StringEnumerationProperty(
, but they are not instrumented, so will not cause API changes.
In the above commits, I changed StringEnumerationProperty to phetioValueType: StringUnionIO
, and regenerated the API file for molecule-polarity.
@samreid please review. Feel free to close, unless you'd like a second look by @zepumph.
Seeing StringEnumerationProperty
and phetioValueType: StringUnionIO
together kind of makes me wonder whether we should either:
(a) rename StringEnumerationProperty to StringUnionProperty, or (b) rename StringUnionIO to StringEnumerationIO
I'd prefer (a). "String union" is more idiomatic TypeScript. And I have been thinking of "enumerations" and "unions" as different things, as do other TypeScript programmers: https://www.bam.tech/article/should-you-use-enums-or-union-types-in-typescript
rename StringEnumerationProperty to StringUnionProperty
That sounds preferable to me
I'll handle the rename of StringEnumerationProperty to StringUnionProperty.
In the above commits, StringEnumerationProperty was renamed to StringUnionProperty.
I think we're done here, so I'll close. If I missed something, or if someone wants to have another look, please reopen.
Do we need an IOType for string unions,
StringUnionIO
? Would there be type-safety benefits? Other benefits?I'm using string unions a lot. And when string unions are involved in IOTypes, I've been using StringIO. But it results in odd code like calculus-grapher CurvePoint.ts shown below. In this example,
pointType: StringIO
seems too permissive. I'd prefer something likepointType: StringUnionIO<PointType>
.