Closed chrisklus closed 1 year ago
GREAT suggestion. There is currently a semantic mismatch between valueType
and phetioType
. So when converting from valueType
to phetioType
, I'm constantly forgetting to add the PropertyIO
or DerivedPropertyIO
wrapper. And it always feels like I'm adding redundant info that could be added by Property
and DerivedProperty
respectively. Based on decisions in https://github.com/phetsims/axon/issues/235, replacing valueType
with phetioType
is the new "standard", so anything that we can do to make that easier is a "win".
DerivedProperty may also need to provide the equivalent of outerTypeIO
or some other way to indicate that its supertype Property should not create PropertyIO
.
I can take this one on.
DerivedProperty may also need to provide the equivalent of outerTypeIO or some other way to indicate that its supertype Property should not create PropertyIO.
Yes this is the same thing we are doing for Emitter/Action, and it works well there.
You can find all the "outer types" by case insensitive search here: new IOType.*Property
. There are currently 5:
DerivedProperty.ts
NumberProperty.ts
Property.ts
InterpolatedProperty.ts
RewindableProperty.ts
We hit this today over in https://github.com/phetsims/studio/issues/253#issuecomment-1101754581. It would be nice to solve this at some point soon. @samreid and I both don't really seem to have the bandwidth, but I'll co-assign him just in case.
Working copy patch:
I'm ready to start committing this change. Local unit tests, lint & type checking are passing. Spot checked sims are running ok in phet brand, phet-io brand and studio. Aqua is looking good so far.
All pushed. Merge conflicts in 3 repos, all resolved. This seems like a good change. Will monitor CT for trouble, but closing for now.
Perhaps It would be worth a review over here. @samreid and I just noticed that we are no longer using NumberPropertyIO, which presumably has changed the API and needs to be fixed.
I fixed the known issue in NumberProperty, over to @zepumph for a closer review.
This seems like the most important part of the code, to catch future issues.
What do you think about this patch?
Index: js/NumberProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/NumberProperty.ts b/js/NumberProperty.ts
--- a/js/NumberProperty.ts (revision 3cdec990c755d9677bba2a5bf98b9e30b49aaa79)
+++ b/js/NumberProperty.ts (date 1664923495533)
@@ -99,6 +99,7 @@
// client cannot specify superclass options that are controlled by NumberProperty
options.valueType = 'number';
options.phetioOuterType = () => NumberProperty.NumberPropertyIO;
+ options.phetioValueType = NumberIO; // not actually used, but for completeness, don't have ReadOnlyProperty storing the wrong default.
const rangePropertyProvided = options.range && options.range instanceof ReadOnlyProperty;
const ownsRangeProperty = !rangePropertyProvided;
I liked the recommendation so much, I went ahead and committed it. Anything else to do for this issue?
Hmmm, now I'm not so sure, but I would like to discuss if this should have caught the error @pixelzoom reported in https://github.com/phetsims/studio/issues/286
I feel that new code is generally done in TypeScript, but it is easy enough to keep around that assertion longer "just in case". Also, https://github.com/phetsims/studio/issues/286 was about a missing phetioValueType
rather than a provided phetioType
so I feel this issue is OK to close. Anyone feel free to disagree/repoen.
@zepumph, @samreid, and @chrisklus think it would be nice to replace
phetioType
withphetioValueType
for Properties. This would eliminate the use of PropertyIO all over in sim code, since it would be part of the default options. From GQModel.js (note: redundant valueType has been deleted for clarity).would become:
This would be a minor change, but we feel that it should be done soon. @pixelzoom what do you think?