Closed pixelzoom closed 10 months ago
@marlitas and I added support for this in the commit.
This issue has good progress and works great to have a state type. I feel the next step for this issue is to remove the defaults for the type parameters: class IOType<T = any, StateType = any>
==> class IOType<T, StateType> {
. When I do this, there are around 100 type errors. So that may be a time-intensive chip-away.
@zepumph and I discussed this and talked about adding StateSchema into this. Keep in mind StateSchema has the two types--composite and value-oriented. This does not need to be done for the milestone.
@zepumph made progress in the commits--I'm not sure what's next or what's left.
Today @samreid mentioned that he would like to see the state type inferred in many cases when creating IOType. It wasn't clear how that would be done, but it would be cool! We should discuss further.
Not sure what else to do here until we work on https://github.com/phetsims/tandem/issues/275. @zepumph can you advise, or can we catch up on it at an upcoming meeting?
I'm not exactly sure. You are probably right. I thought for a time that we were heading to a spot where the parameter types were going to be required, but that seems less than ideal. Let's make sure that we are treating https://github.com/phetsims/tandem/issues/275 as our primary issue for investigation of IOType improvements.
Lots of good progress today here because of https://github.com/phetsims/greenhouse-effect/issues/361. We found that TypeScript was really lacking in its ability to type check based on the state type provided to the IOType. @samreid and I fixed this! Or at least a large portion of it. It is passing type checking and cleaned up a bunch of good stuff. @samreid I wonder how much more is needed for this issue.
IOType is currently loaded with uses of any
Much of this was removed in commits below https://github.com/phetsims/tandem/issues/261#issuecomment-1185001659.
https://github.com/phetsims/tandem/issues/305 will remove the default IOType parameters (anys).
We will close this once we confirm that CT is clear.
CT looks normal to me. @zepumph want to double check before we close?
On 6/6/2022, @marlitas initiated a discussion in Slack#typescript about IOType. She was trying to eliminate uses of
any
in mean-share-and-balancePipeModelIO
. IOType is currently loaded with uses ofany
, and is lacking in ability to provide type-checking. The conclusion was that adding a second parameter variable to IOType would provide more type safety.For example:
class IOType<T, S>
where
T
is the class of the object being serialized, andS
is the type of the state object.Then in IOType.ts
... and perhaps some others
Slack#typescript discussion:
Marla Schulz 3:20 PM Attempting to fix
no-explicit-any
in the Mean: Share and Balance repo, and I don't understand PhetIO enough to figure out what type the stateObject in the below code snippet should be. Is there a rule of thumb or reference I could look at ?Chris Malley 3:23 PM IOType.ts is littered with any, so I’m not sure if there’s an answer to this yet. :+1:
Chris Malley Today at 3:24 PM IOType.ts line 83: readonly stateToArgsForConstructor: any; (edited)
Marla Schulz 1 hour ago Okay Gotcha. I'll hold off on this then...
Chris Malley 1 hour ago
readonly stateToArgsForConstructor: any => any
would have at least ensured that it’s a function with 1 parameter that returns something.Chris Malley 1 hour ago The return type of
toStateObject
and parameter forstateToArgsForConstructor
probably both need to be something like:… but (1) I don’t know if the plan is to define a type for them, and (2) I don’t know how this relates to
stateSchema
.Chris Malley 1 hour ago
PipeModelStateObject
andstateSchema
unfortunately both specify the same thing, but in different ways, for different purposes. The former for type-checking, the latter for PhET-iO.Chris Malley 1 hour ago Here’s what I came up with. Yuck.
Chris Malley 1 hour ago But this doesn’t really provide any type checking. Add
z: number to PipeModelStateObject
, and you’ll get no complaint about the return type oftoStateObject
, etc.Marla Schulz 1 hour ago Oh Gosh... so the stateObject is just an object that contains any instance variables that need to be tracked by PhetIO?
Chris Malley 1 hour ago Yes. It’s the things that will be saved (serialized) and restored (deserialized). (edited)
Marla Schulz 1 hour ago Perhaps this is a temporary workaround? We can set immutable object types right? Kind of the same thing we do with options?
Chris Malley 1 hour ago I’d wait for an answer from the PhET-iO subteam. I’m going to be bumping into this very soon for MOTHA, where I’ll need custom IOTypes and PhetioGroup. :+1:
Michael Kauzmann 1 hour ago We will likely not have time for an investigation. In Ratio and Proportion I felt good about getting rid of the any in exchange for the type-out state object literal, see https://github.com/phetsims/ratio-and-proportion/blob/0bfef28eb3ba5bdb051bc5183786c508e89d62da/js/common/rapConstants.ts#L20-L30. I would recommend doing that here too to unblock:
type StateObject = { x number; y: number; };
Chris Malley 1 hour ago IOTypes is currently:
and your usage is:
It seems like it would better to do:
and then have IOType define (e.g.)
Michael Kauzmann 1 hour ago In a meeting.
Chris Malley 1 hour ago Just pointing out that in RAP (rapConstants.ts) you have:
That’s 100% duplication, and a lot of opportunity for them to get out-of-sync. Especially since you’ve defined them 50 lines away from each other.
Sam Reid:house_with_garden: 1 hour ago I don’t see an obvious way to improve over the “Here’s what I came up with. Yuck.” code snippet.
Chris Malley 1 hour ago Do you mean right now, or long term?
Sam Reid:house_with_garden: 1 hour ago Except maybe to also specify the return type of
toStateObject
to also bePipeModelStateObject
Chris Malley 1 hour ago MK has additionally specified the return type of
toStateObject
in his rapConstants.ts example.Sam Reid:house_with_garden: 1 hour ago Nice
Chris Malley 45 minutes ago But to summarize the problems:
toStateObject
return value andstateToArgsForConstructor
param.Chris Malley 40 minutes ago A couple of these could be addressed by giving IOType two parameter variables:
class IOType<T, S>
whereT
is the class of the object being serialized, andS
is the type of the state object.Then you’d have in IOType.ts:
Chris Malley 34 minutes ago Then @mjkauzmann example would be:
And @Marla Schulz example would be:
Chris Malley 32 minutes ago
valueType: PipeModel
feels redundant in @Marla Schulz case too. But I guess that can’t be helped. (edited)Sam Reid:house_with_garden: 33 minutes ago That 2nd type parameter for state type looks great