Closed pixelzoom closed 1 year ago
@zepumph and I also discussed adding this type:
Record<string,IOType | Record<string|IOType>>
Not perfect, but would help get us past some ts-ignores.
Also wanted to note this patch which we could pursue later, but I like the above to start.
I should also point out that the _private
convention in StateSchema does not seem to be well-known, and therefore not consistently followed. For example, in greenhouse Wave.ts, WaveIO does not use _private
to serialize the private fields of Wave (propagationLimit
, renderingWavelength
, etc.). @jbphet
@samreid and @zepumph - I was mentioned above in this issue, so now I'm wondering whether I should modify the code in greenhouse-effect to use _private
. What do you recommend?
Does Greenhouse PhET-iO state data have values that are more of an "implementation detail" than a "user-facing feature"? If so, those values should be nested under _private
.
I don't want to hijack this issue with questions specific to Greenhouse Effect, so I've set up an issue where I'll try to get information on how to use _private
appropriately in that sim.
This issue is identified as one of the many problems with BunnyIO in https://github.com/phetsims/natural-selection/issues/327.
Elevating the priority of this by labeling for PhET-iO meeting. I can't properly specify the StateSchema for my IOTypes in natural-selection until this is resolved. Using @ts-expect-error
in multiple places in the meantime.
More work on this today, but I couldn't crack it. https://github.com/Microsoft/TypeScript/issues/29023 https://stackoverflow.com/questions/24613955/is-there-a-type-in-typescript-for-anything-except-functions were helpful in telling me that typescript may not be able to assist us.
... typescript may not be able to assist us.
It's not only assisting us, it's hindering us in this case.
If the _private
pattern is going to be problematic with TypeScript, should we consider abandoning it? And brainstorm different patterns that would work with TypeScript?
Here's one idea: Flatten all StateObject types, since the nesting is presumably the problem. Treat fields in StateObject types that begin with "_" as private, and do not display them in Studio. For example:
type MyStateObject = {
x: number;
y: number;
- _private: {
- color: TColor
- };
+ _color: TColor;
};
One disadvantage is that StateObject field names may not match core-type field names, so default behavior of IOType that relies on stateSchema
may not work. (Maybe that's a problem with _private
too? I don't know...)
Other ideas?
That is a reasonable proposal and it seems good. I don't know if there will be difficulty in implementing that and it would probably be good to investigate before alternatives. If it doesn't work out, other options could be like:
private static getStateSchema( BunnyIO: IOType ): CompositeSchema {
return {
public: {
father: NullableIO( ReferenceIO( BunnyIO ) ),
age: NumberIO
},
private: {
hopDelta: Vector3.Vector3IO,
hopStartPosition: Vector3.Vector3IO
}
};
}
Or
private static getStateSchema( BunnyIO: IOType ): CompositeSchema {
return {
father: NullableIO( ReferenceIO( BunnyIO ) ),
age: NumberIO,
hopDelta: private( Vector3.Vector3IO ),
hopStartPosition: private( Vector3.Vector3IO )
};
}
Current progress, seems very promising:
I feel like this issue should be addressed as part of an iteration goal. So I'll self-unassign and add it to the proposed backlog issues for next iteration.
This will likely need a migration rule since BunnyIO state currently looks like this:
More progress but not running yet. Should I be in a branch?
@samreid and I got to a commit point, but will hold out until a couple of RC shas are taken for high priority sims:
I messaged @pixelzoom and @jessegreenberg and our slack developer channel:
@zepumph and I really want to push major improvements and bugfixes for https://github.com/phetsims/tandem/issues/282 but we don’t want to sneak that in right before RCs are created. Please keep us posted when the RCs are all made.
@pixelzoom reported CG 1.0 branch was created, so this is no longer on hold.
I addressed a remaining TODO in the code, and regenerated API files. I was surprised to see a change in the calculus grapher API. This was a more risky change, and I'll message slack about it, and close.
Seems like CT problems, see https://github.com/phetsims/wave-on-a-string/issues/155
https://github.com/phetsims/wave-on-a-string/issues/155 addressed separately. I skimmed the latest CT column and saw problems like Uncaught Error: Assertion failed: Disposable.dispose() call is missing from an overridden dispose method
which I mentioned in the slack CT channel. Closing.
Related to https://github.com/phetsims/natural-selection/issues/326 ...
When creating an IOType, specifying a
stateSchema
with a_private
field will save the state for those fields, but exclude them from display in Studio. This feature works in JavaScript, but apparently does not have proper TypeScript support. There are currently no TypeScript sims using_private
, and natural-selection is the first to do so.For example, in natural-selection Wolf.ts:
Line 157 is flagged with:
@samreid and I looked at IOType.ts and StateSchema.ts, but could not figure out how to support
_private
.@zepumph could you please take a look?