Create a TModel type alias #783
Comments
|
Maybe this get's a reset function too? |
|
I committed IModel today for a different issue. But it doesn't have a reset method yet. |
|
I don't see any calls to UPDATE: Tagging for phetsims/tasks#1111 |
|
In 0d6f15d I made IModel the default for Screen, which was nice to not have to give it parameters when really it is quite general. |
|
Based on conclusions in phetsims/chipper#1283, and @samreid's proposal to ban |
|
It is: Line 11 in 791c659 |
zepumph
added a commit
to phetsims/bamboo
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/quadrilateral
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/mobius
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/sun
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/wilder
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/models-of-the-hydrogen-atom
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/bending-light
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/gravity-and-orbits
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/circuit-construction-kit-common
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/nitroglycerin
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/scenery-phet
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/number-play
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
to phetsims/vegas
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
zepumph
added a commit
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, #783
zepumph
added a commit
to phetsims/chipper
that referenced
this issue
Jul 19, 2022
…btypes as parameters to createView, update typescript to 4.7, phetsims/joist#783
|
I can't say that I'm wild about the approach in the patch in #783 (comment). But I don't have a better suggestion. EDIT: I really dislike this bit. And is it correct? +export default class Sim<M1 extends IModel, V1 extends ScreenView,
+ M2 extends IModel, V2 extends ScreenView,
+ M3 extends IModel, V3 extends ScreenView,
+ M4 extends IModel, V4 extends ScreenView,
+ M5 extends IModel, V5 extends ScreenView> extends PhetioObject {
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ], providedOptions?: SimOptions );
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ] | [ Screen<M1, V1>, Screen<M2, V2> ], providedOptions?: SimOptions );
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ] | [ Screen<M1, V1>, Screen<M2, V2> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3> ], providedOptions?: SimOptions );
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ] | [ Screen<M1, V1>, Screen<M2, V2> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3>, Screen<M4, V4> ], providedOptions?: SimOptions );
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ] | [ Screen<M1, V1>, Screen<M2, V2> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3>, Screen<M4, V4> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3>, Screen<M4, V4>, Screen<M5, V5> ], providedOptions?: SimOptions );
+ public constructor( name: string, allSimScreens: [ Screen<M1, V1> ] | [ Screen<M1, V1>, Screen<M2, V2> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3>, Screen<M4, V4> ] | [ Screen<M1, V1>, Screen<M2, V2>, Screen<M3, V3>, Screen<M4, V4>, Screen<M5, V5> ], providedOptions?: SimOptions ) {That said... If
|
The proposed strategy gets type inference to work well in all sites that use const sim = new Sim( fourierMakingWavesTitleString, [
new DiscreteScreen( { tandem: Tandem.ROOT.createTandem( 'discreteScreen' ) } ),
new WaveGameScreen( { tandem: Tandem.ROOT.createTandem( 'waveGameScreen' ) } ),
new WavePacketScreen( { tandem: Tandem.ROOT.createTandem( 'wavePacketScreen' ) } )
], simOptions );
sim.start();
I skimmed it and did not see any obvious defects. Let's discuss further before proceeding. |
|
@samreid let me know when you'd like to discuss. |
|
I added a note to my calendar to reach out to @pixelzoom in the coming week. Self-unassigning until then. UPDATE: Perhaps an on-hold label would be good instead. |
|
Removing the hold to update the patch before discussing with @pixelzoom |
|
Current patch: |
|
Discussed with @samreid. There's definitiely something odd going on here, or maybe something about TypeScript that we don't understand. But this seems like lower prioirty at the moment, so I recommend keepin the ts-ignore in Screen.ts, with a link to this issue. I.e.: // @ts-ignore
type CreateView<out M extends TModel, V> = ( model: M ) => V;@samreid please feel free to add additional notes that might be helpful in the future, then feel free to label as deferred. |
|
Summarizing key points from my discussion with @pixelzoom:
// @ts-ignore
type CreateView<out M extends TModel, V> = ( model: M ) => V;
This is a buggy type annotation because The problem is that Sim only knows that it has a And still has many type errors left to correct. Those changes are nice because it distinguishes between sim screens and the homescreen, but there is just too much overhead to justify it. The other problem @pixelzoom and I observed is that Our conclusion is that we think if we had better TypeScript expertise, there would be a more elegant solution, perhaps using a more array-like strategy for the Sim's individual Screen types. But for now, we would like to shut off this error with an isolated |
samreid
added a commit
that referenced
this issue
Aug 30, 2022
|
I changed from the incorrect variance annotation to one IntentionalAny in the commit. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This will have a
stepfunction. For improving upon type safety and documentation. It also allows for a space to expand the API in the future.phetsims/ratio-and-proportion#405
The text was updated successfully, but these errors were encountered: