zepumph
commented
2 years ago Maybe this get's a reset function too?
Open zepumph opened 2 years ago
zepumph
commented
2 years ago Maybe this get's a reset function too?
samreid
commented
2 years ago I committed IModel today for a different issue. But it doesn't have a reset method yet.
samreid
commented
2 years ago I don't see any calls to reset in joist, so I'm not sure the advantage of adding that as an optional part of the interface. @zepumph can this be closed?
UPDATE: Tagging for https://github.com/phetsims/tasks/issues/1111
zepumph
commented
2 years ago In https://github.com/phetsims/joist/commit/0d6f15d4ba1fc319902af99ad6496ba6ea7ed36e I made IModel the default for Screen, which was nice to not have to give it parameters when really it is quite general.
pixelzoom
commented
2 years ago Based on conclusions in https://github.com/phetsims/chipper/issues/1283, and @samreid's proposal to ban interface... Perhaps type should be used instead of interface.
zepumph
commented
2 years ago zepumph
commented
2 years ago I committed with @samreid today, we were able to use out as part of 4.7 to get IModel to typecheck even when a parameter to a passed in function. I'd like to mark this for dev meeting though as a PSA about a couple things:
ts-ignore for out case, so we are still figuring them out.step function, it was pretty straightforward to add, and was (in our opinion) the easiest way to structurally type IModel in way that didn't error out lint and tsc.pixelzoom
commented
2 years ago This is failing for me in a couple of places, and I'm dead in the water.
In Screen.ts, both WebStorm and tsc are complaining about the definiton of CreateView, despite the ts-ignore:
In Screen subclasses like LensScreen.ts, I'm seeing an "implicit any" error in WebStorm:
samreid
commented
2 years ago I messaged slack:
We have updated to TypeScript version 4.7, please see https://github.com/phetsims/joist/issues/783#issuecomment-1189650113 for context and detail. Next time you pull, please npm install in chipper.
As mentioned in https://github.com/phetsims/chipper/issues/1212#issuecomment-1137922311 this requires WebStorm 2022.1.2 RC or higher.
pixelzoom
commented
2 years ago Upgrading to TS 4.7 resolved the issues I reported in https://github.com/phetsims/joist/issues/783#issuecomment-1189667682.
samreid
commented
2 years ago I investigated how to implement the IModel constraint without any ts-ignore. The problem is that Sim and Screen specify IModel as a parameter in createModel, so when a Screen is parameterized as MyModel => MyView it needs MyModel =>MyView and will not accept IModel => MyView because that type parameter already told it to expect something more specific.
Brainstorming ideas:
in out on the generic if we also put the model type as a return type, like CreateView returns [M,V] instead of just V. However, this is not type safe and would lead to a horrible API.out on the contravariant generic is also not type safe and IMO is worse than just doing a ts-ignore.I followed the philosophy from DerivedProperty, which is repetitive and explicit. However, it doesn't work as well here since we can't always use inference and it is unfortunate to have to repeat ourselves with type parameters.
If we (a) use inference at all call sites and (b) invent a type like UnknownSim where it knows everything but the model types, then this could be doable. But maybe it would be better to put selective ts-ignore and transform a type safety problem into a runtime check.
samreid
commented
2 years ago At today's developer meeting, we agreed the final proposal and patch in the about bullet point sounds promising, but we want to run it past @pixelzoom before implementing it.
pixelzoom
commented
2 years ago I can't say that I'm wild about the approach in the patch in https://github.com/phetsims/joist/issues/783#issuecomment-1189781470. 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 create is the new way of instantiating Sim, then Sim's constructor should be protected.
IModel should be renamed to TModel, or perhaps something without a prefix. (I don't feel strongly about eradicating the 'T' prefix.)
samreid
commented
2 years ago If create is the new way of instantiating Sim, then Sim's constructor should be protected.
The proposed strategy gets type inference to work well in all sites that use new Sim. For instance, in Fourier, it could infer all the model and view types from this part:
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();Sim.create was our workaround for sites that extend Sim where inference cannot be used. But maybe it would be better if all sites could use new Sim.
EDIT: I really dislike this bit. And is it correct?
I skimmed it and did not see any obvious defects.
Let's discuss further before proceeding.
pixelzoom
commented
2 years ago @samreid let me know when you'd like to discuss.
samreid
commented
2 years ago 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.
samreid
commented
2 years ago Removing the hold to update the patch before discussing with @pixelzoom
samreid
commented
2 years ago Current patch:
pixelzoom
commented
2 years ago 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.
samreid
commented
2 years ago Summarizing key points from my discussion with @pixelzoom:
// @ts-ignore
type CreateView<out M extends TModel, V> = ( model: M ) => V;/Users/samreid/apache-document-root/main/joist/js/Screen.ts(73,17): error TS2636: Type 'CreateView<sub-M, V>' is not assignable to type 'CreateView<super-M, V>' as implied by variance annotation.
Types of parameters 'model' and 'model' are incompatible.
Type 'super-M' is not assignable to type 'sub-M'.This is a buggy type annotation because M is in the in location, and out is not correct. Removing the variance annotation like so: type CreateView<M extends TModel, V> = ( model: M ) => V; gives 73 errors in sim code like:
/Users/samreid/apache-document-root/main/geometric-optics/js/GOSim.ts(58,7): error TS2322: Type 'LensScreen' is not assignable to type 'Screen<TModel, ScreenView>'.
Types of property 'createView' are incompatible.
Type 'CreateView<LensModel, LensScreenView>' is not assignable to type 'CreateView<TModel, ScreenView>'.
Type 'TModel' is missing the following properties from type 'LensModel': lens, reset, opticalObjectChoiceProperty, raysTypeProperty, and 12 more.The problem is that Sim only knows that it has a TModel. So type checking createView(LensModel) fails because it doesn't know for sure that it is a LensModel. That is why the proposal in https://github.com/phetsims/joist/issues/783#issuecomment-1230800671 is to add more type information to Sim so it knows what the sim model and view types are. However, that patch leads to complex, verbose and redundant code like:
+export default function selectScreens<M1 extends TModel, V1 extends ScreenView, M2 extends TModel, V2 extends ScreenView, M3 extends TModel, V3 extends ScreenView, M4 extends TModel, V4 extends ScreenView, M5 extends TModel, V5 extends ScreenView, M6 extends TModel, V6 extends ScreenView>(
+ allSimScreens: ( Screen<M1, V1> | Screen<M2, V2> | Screen<M3, V3> | Screen<M4, V4> | Screen<M5, V5> | Screen<M6, V6> )[],
+ homeScreenQueryParameter: boolean,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 Screen<TModel,ScreenView> could not accept a value of HomeScreen<HomeScreenModel,HomeScreenView> even though HomeScreenModel implements TModel and HomeScreenView extends ScreenView. This is likely because HomeScreenModel appears in the contravariant position and the view appears in the covariant position.
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 ts-ignore or any or something. The existing commit does that (in an awkward way).
samreid
commented
2 years ago I changed from the incorrect variance annotation to one IntentionalAny in the commit.
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.https://github.com/phetsims/ratio-and-proportion/issues/405