Open pixelzoom opened 1 year ago
I should also point out that toStateObject
and fromStateObject
also contain a form of duplication. For ChallengeIO:
public toStateObject(): ChallengeStateObject {
return {
x: NumberIO.toStateObject( this.x ),
a: Fraction.FractionIO.toStateObject( this.a ),
b: Fraction.FractionIO.toStateObject( this.b ),
m: Fraction.FractionIO.toStateObject( this.m ),
n: Fraction.FractionIO.toStateObject( this.n ),
debugDerivation: StringIO.toStateObject( this.debugDerivation )
};
}
public static fromStateObject( stateObject: ChallengeStateObject ): Challenge {
return new Challenge(
NumberIO.fromStateObject( stateObject.x ),
Fraction.FractionIO.fromStateObject( stateObject.a ),
Fraction.FractionIO.fromStateObject( stateObject.b ),
Fraction.FractionIO.fromStateObject( stateObject.m ),
Fraction.FractionIO.fromStateObject( stateObject.n ),
stateObject.debugDerivation
);
}
So in general, if I change a field, I need to change it in 5 places, not 3 places.
I took a look at this, and tried to consolidate types. Here is a non-running patch:
This patch includes this new type:
type StateObject<M extends Record<string, IOType>> = {
[Key in keyof M]: ReturnType<M[Key]['toStateObject']>;
};
This takes a StateSchema and gets all of its state types. This works well for terminal/primitive states, but it doesn't handle recursively/nested composite types.
But for cases like this, it allows us to infer that state type from a schema:
export type ChallengeStateObject = StateObject<typeof stateSchema>;
I also noticed StateSchema doesn't support defaultFromStateObject
but if implemented, it could be used like so:
public static fromStateObject( stateObject: ChallengeStateObject ): Challenge {
const coreObject = StateSchema.defaultFromStateObject( stateObject );
return new Challenge( coreObject.x, coreObject.a, coreObject.b, coreObject.m, coreObject.n, coreObject.debugDerivation );
}
After these changes, the central type is the StateSchema. StateObject type is derived from that. Still have to unpack things to call a constructor.
This doesn't seem higher priority that existing milestones, but I wanted to jot some thoughts down. A good next step would be discussion/feedback with @zepumph
Ok. I actually really like @samreid's idea in the above patch, but I am getting some really weird lack of errors from typescript. I created a strict version that doesn't let IntentionalAny leak into state object types, but it doesn't type check on never
as I would have expected. @samreid can you apply the patch and look at that TODO? I'm not really trusting my system type checking right now.
@zepumph and I worked on this yesterday and I thought we wrote a message that we were at a good point to check in with @pixelzoom. But I'm not sure if I'm thinking of another issue.
Basically we asking for feedback on getting rid of one of the three descriptions.
In this pattern we pull out the stateSchema object from the IOType option, and then use it as a const to get the typescript type of the state object for it. I just committed the same change to Vector2 so that you could see another example of the conversion.
@zepumph said:
Basically we asking for feedback on getting rid of one of the three descriptions. ...
I've read the comments several times, looked at the commits, and looked at Vector2IO. I can't decipher which of "the three descriptions" you're proposing to get rid of, or how this affects writing IOTypes in general.
Encountered while instrumenting equality-explorer for https://github.com/phetsims/equality-explorer/issues/192. And I've encountered this before, but TypeScript has made the problem worse, so I'm finally creating an issue.
There is an undesirable amount of duplication involved in definiting an IOType. For example, let's look at equality-explorer Challenge.ts, where
ChallengeIO
is defined.First, there are the fields in the core type. In the case of
Challenge
, I have 6 fields, defined using constructor assignment shorthand:Second, there's the StateObject type, in this case
ChallengeStateObject
. Here I have the same 6 fields, but they need to be defined asStateObject
types (withStringIO
being the exception, which is a different problem, see https://github.com/phetsims/tandem/issues/277.)Third, there's the definition of
stateSchema
for the IOType, in this caseChallengeIO
. Again I have the same 6 fields, but here they need to defined asIOTypes
:So in general, if I have N fields to serialize, I must describe those fields in 3 different ways:
And as I make changes, I need to make changes in 3 places, and use the correct type in those places. That's a lot of undesirable duplication, and a lot to keep track of.
Is there something that can be done to improve this?