Closed samreid closed 1 year ago
pixelzoom
commented
1 year ago
- Marking IOType as an interface
Why should IOType be an interface, rather than a type?
In https://github.com/phetsims/chipper/issues/1280, https://github.com/phetsims/phet-info/issues/200, we concluded that PhET should prefer type over interface. Also in that issue, we converted many (all?) interfaces to types. See also https://github.com/phetsims/phet-info/issues/200.
samreid
commented
1 year ago Thanks, I updated the title and text.
samreid
commented
1 year ago To support https://github.com/phetsims/tandem/issues/275, how can I say that an abstract class has a subclass that returns instances that match an interface? So far I have:
type HasName = { getName: () => string };
export type CreatesHasName = { new( ...args: IntentionalAny[] ): HasName; };
class Person implements HasName { public constructor() {
// blank}
public getName(): string { return 'Me!!'; } }
const P: CreatesHasName = Person;
console.log( P ); This all works great, but if I mark class Person as abstract, then I get this error:
TS2322: Type ‘typeof Person’ is not assignable to type ‘CreatesHasName’. Cannot assign an abstract constructor type to a non-abstract constructor type.
Maybe this will do it? https://stackoverflow.com/questions/36886082/abstract-constructor-type-in-typescript
export type CreatesHasName = abstract new( ...args: IntentionalAny[] ) => HasName;What is preferable, and least-surprise. Type parameters always appear in the same order? Or each set of type parameters makes the most sense for its case? Which is preferable, Directory1 or Directory2?
type GuidanceCounselor<Person, Career> = {
getCareer: ( p: Person ) => Career;
};
type Directory1<Person, Career> = {
getPerson: ( c: Career ) => Person;
};
type Directory2<Career, Person> = {
getPerson: ( c: Career ) => Person;
};@pixelzoom replied:
Whatever makes sense for the thing being parameterized. In your example, I can’t tell if there’s any relationship between GuidanceCounselor and Directory. If there’s no relationship, then the order of Directory parameters should definitely not be affected by the order of GuidanceCounselor parameters. If there is a relationship, I still think it’s preferable to use what is most appropriate for each class. For Directory1 and Directory2 specifically, I have a slight preference for Directory1, because a Person has a Career. I don’t think it’s significant (or necessary) that the left-to-right order of generic params matches their left-to-right use by getPerson. There could be other things that are parameterized in those classes that doesn’t match that order.
samreid
commented
1 year ago Current progress, IOType looks like this:
// Copyright 2020-2022, University of Colorado Boulder
/**
* IO Types form a synthetic type system used to describe PhET-iO Elements. A PhET-iO Element is an instrumented PhetioObject
* that is interoperable from the "wrapper" frame (outside the sim frame). An IO Type includes documentation, methods,
* names, serialization, etc.
*
* @author Sam Reid (PhET Interactive Simulations)
*/
import TandemConstants, { PhetioObjectMetadata } from '../TandemConstants.js';
import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js';
export type IOTypeMethod = {
returnType: IOType;
parameterTypes: IOType[];
//the function to execute when this method is called. This function's parameters will be based on `parameterTypes`,
// and should return the type specified by `returnType`
implementation: ( ...args: IntentionalAny[] ) => unknown;
documentation: string;
// by default, all methods are invocable for all elements. However, for some read-only elements, certain methods
// should not be invocable. In that case, they are marked as invocableForReadOnlyElements: false.
invocableForReadOnlyElements?: boolean;
};
type IOType = {
typeName: string;
supertype?: IOType | null;
events?: string[];
dataDefaults?: Record<string, unknown>;
metadataDefaults?: Partial<PhetioObjectMetadata>;
documentation?: string;
methods?: Record<string, IOTypeMethod>;
methodOrder?: string[];
parameterTypes?: IOType[];
isFunctionType?: boolean;
};
export default IOType;
export const ObjectIO: IOType = {
typeName: TandemConstants.OBJECT_IO_TYPE_NAME,
supertype: null,
documentation: 'The root of the IO Type hierarchy',
metadataDefaults: TandemConstants.PHET_IO_OBJECT_METADATA_DEFAULTS
};
// Static methods
export type FromStateObject<T, S> = { fromStateObject: ( s: S ) => T };
export type StateToArgsForConstructor<S> = { stateToArgsForConstructor: ( s: S ) => unknown[] }; // TODO: instead of unknown this is the second parameter type for PhetioDynamicElementContainer. How? https://github.com/phetsims/tandem/issues/261
export type HasStateSchema = {
stateSchema: Record<string, IOType>;
};
// These methods appear on the instance
export type ToStateObject<T> = { toStateObject: () => T };
export type ApplyState<S> = { applyState: ( s: S ) => void };
export type AddChildElement<T, S> = { addChildElement: ( componentName: string, s: S ) => T };
export type ConstructsToStateObject<T, S> = abstract new( ...args: IntentionalAny[] ) => ToStateObject<S>;
export type ConstructsApplyState<T, S> = abstract new( ...args: IntentionalAny[] ) => ApplyState<S>;
export type ConstructsAddChildElement<T, S> = abstract new( ...args: IntentionalAny[] ) => AddChildElement<T, S>;
export type ReferenceIOType<T, S> = ConstructsToStateObject<T, S> & ConstructsApplyState<T, S> & IOType;And LayersModel in greenhouse will change from:
public static readonly LayersModelIO: IOType = new IOType( 'LayersModelIO', {
valueType: LayersModel,
stateSchema: LayersModel.STATE_SCHEMA,
toStateObject: ( a: LayersModel ) => a.toStateObject(),
applyState: ( a: LayersModel, b: LayersModelStateObject ) => a.applyState( b )
} );to
public static readonly LayersModelIO: ReferenceIOType<LayersModel, LayersModelStateObject> = LayersModel;
public static typeName = 'LayersModelIO';
public static stateSchema = {
emEnergyPackets: ArrayIO( EMEnergyPacket.EMEnergyPacketIO )
};I'm not clear on the changes in greenhouse above. These are static fields in what class? And does that class now implement IOType?
Also wondering if this major change should be reviewed by @zepumph before being pushed to all PhET-iO sims. Or maybe this is something that you've already discussed with him.
samreid
commented
1 year ago I'm not clear on the changes in greenhouse above. These are static fields in what class?
LayersModel. Here is a patch with more context:
And does that class now implement IOType
No, the proposal is that classes would not implement IOType, but that typeof MyClass implements IOType. That is because some of the parts, such as typeName, documentation, events, etc should be shared between all instances. That type is expressed on this line:
LayersModelIO: ReferenceIOType<LayersModel, LayersModelStateObject> = LayersModel;Also wondering if this major change should be reviewed by @zepumph before being pushed to all PhET-iO sims. Or maybe this is something that you've already discussed with him.
Yes, I labeled it "epic" and it would be good to discuss.
samreid
commented
1 year ago There is already something very much like this called PhetioType in TandemConstants:
export type PhetioType = {
methods: Methods;
supertype?: string; // no supertype for root of hierarchy
typeName: string;
documentation?: string;
events: string[];
metadataDefaults?: Partial<PhetioObjectMetadata>;
dataDefaults?: Record<string, unknown>;
methodOrder?: string[];
stateSchema?: PhetioObjectState;
parameterTypes?: string[]; // each typeName
};
zepumph
commented
1 year ago Very interesting stuff! I like it. Can we discuss sometime soon?
samreid
commented
1 year ago I reached out to @zepumph and we are hoping to discuss today. Here's my current patch for my reference:
samreid
commented
1 year ago At today's quarterly planning meeting, we acknowledged that even though this may be a good long-term direction, we need to focus on other priorities this quarter. Unassigning for now.
samreid
commented
1 year ago The cons about excess property checking sound problematic. I don't think this proposal is the way of the future. Closing.
Starting with https://github.com/phetsims/tandem/issues/263 and its side issues https://github.com/phetsims/tandem/issues/274 and https://github.com/phetsims/tandem/issues/273, and related to https://github.com/phetsims/tandem/issues/261, I would like to experiment with combining IO Type information into the core classes. Now that we have TypeScript, this can be accomplished by:
This is also partially in hopes to address the dev team's expression of difficulty in using IO Types. @pixelzoom and @jbphet mentioned difficulty with IO Types yesterday, and @zepumph and I have previously had discussions about whether IO Types should be more combined with the core types.
Here are the known pros and cons of the proposed approach:
Pros:
Electron: ReferenceDataIOType = ...rather than trying to infer it from various options. Likewise, avoid the effort, complexity and API complexity of having a mutually exclusive serialization type detection from provided options.Cons:
const CircuitElement: IOType = class extends PhetioObject {works OK butconst CircuitElement: IOType = abstract class extends PhetioObject {is a syntax error.~ UPDATE: It works if you declare the const on a separate line.