Closed pixelzoom closed 1 year ago
(1) Developers are not using TModel for their top-level model classes. See for example https://github.com/phetsims/calculus-grapher/issues/117. See also GravityAndOrbitsModel (@samreid), and many other examples.
GravityAndOrbitsModel implements TModel because it matches the interface. TypeScript is structurally typed rather than nominally typed, so because GravityAndOrbitsModel
matches the TModel
interface, it can be used where TModel
is expected. I do not see the necessity to change class GravityAndOrbitsModel
to class GravityAndOrbitsModel implements TModel
if that's what you are advocating.
(3) Where developer are parameterizing their Screen subclass, the model M does not implement TModel, but tsc does not complain about this error.
If I remove the reset
method from that hierarchy, then tsc complains because PhotonsModel
no longer satisfies the TModel
requirements.
(2) Developers are not parameterizing their Screen subclasses. See for example https://github.com/phetsims/calculus-grapher/issues/117.
True, but I'm not sure what kind of problems that could create.
@samreid why do you think this is an appropriate use of IntentionalAny?
The code comment says:
// The IntentionalAny in the model type is due to https://github.com/phetsims/joist/issues/783#issuecomment-1231017213
class Screen<M extends TModel = TModel, V extends ScreenView = ScreenView> extends PhetioObject {
which refers to: https://github.com/phetsims/joist/issues/783#issuecomment-1231017213
I tested class Screen<M extends TModel = TModel
but it seemed very nontrivial to add correct typing elsewhere that would make that work.
TypeScript is structurally typed rather than nominally typed, so because GravityAndOrbitsModel matches the TModel interface, it can be used where TModel is expected.
That's a poor reason to omit implements TModel
. If I followed that reasoning, why would I ever use implements
? And why does TypeScript include it?
I do not see the necessity to change
class GravityAndOrbitsModel
toclass GravityAndOrbitsModel implements TModel
if that's what you are advocating.
That is what I was advocating. Yeah, it'll work if you omit implements TModel
, because of structural typing. But if some part of the TModel API is omitted/wrong in GravityAndOrbitsModel, it's going to show errors at usage sites, not in the definition of GravityAndOrbitsModel. And if TModel changes in the future, you'll get little help from TypeScript identifying what needs to be updated.
And it sounds like you're using IntentionalAny because doing it correctly is too much work. Or am I summarizing incorrectly?
Changing
class Screen<M extends TModel = IntentionalAny, V extends ScreenView = ScreenView> extends PhetioObject {
to
class Screen<M extends TModel, V extends ScreenView = ScreenView> extends PhetioObject {
Introduces 80 type errors. I'm not worried about the sim-specific ones. But the ones in Joist would be much more difficult to fix.
And it sounds like you're using IntentionalAny because doing it correctly is too much work.
That is a fair assessment, and in line with our philosophy about getting added value from TypeScript, without spending excessive time to get everything to be type-perfect.
... Introduces 80 type errors. I'm not worried about the sim-specific ones. But the ones in Joist would be much more difficult to fix.
The change that you proposed was as follows, and it introduces 48 TS errors in joist.
class Screen<M extends TModel, V extends ScreenView = ScreenView> extends PhetioObject {
Changing to this reduces the number of TS errors to 27:
class Screen<M extends TModel = TModel, V extends ScreenView = ScreenView> extends PhetioObject {
4 js/NavigationBar.ts:82
1 js/PhetButton.ts:142
1 js/ScreenSelectionSoundGenerator.ts:25
3 js/selectScreens.ts:129
14 js/selectScreensTests.ts:115
4 js/Sim.ts:526
Inspecting the above errors, 100% of them are related to HomeScreen. So there is undoubtedly a type problem with HomeScreen that should be addressed. And I don't see any reason to mask it with IntentionalAny.
Back to @samreid for comment.
There appear to be only 2 unique TS errors related to HomeScreen. Here they are, with examples:
// selectScreens.ts
// TS2345: Argument of type 'HomeScreen' is not assignable to parameter of type 'Screen '.
screens.unshift( homeScreen );
// PhetButton.ts
// TS2367: This comparison appears to be unintentional because the types 'Screen ' and 'HomeScreen | null' have no overlap.
const showHomeScreen = screen === sim.homeScreen;
I'd start with this:
- class HomeScreenModel {
+ class HomeScreenModel implements TModel {
Here's my initial start on this issue:
After this change, there are 101 type errors:
In 1e5ca9da03bd7ba5049ea8b3ccca308a3054b07f, @samreid made this change in Screen.ts:
I'm seeing 3 problems related to this:
(1) Developers are not using TModel for their top-level model classes. See for example https://github.com/phetsims/calculus-grapher/issues/117. See also GravityAndOrbitsModel (@samreid), and many other examples.
(2) Developers are not parameterizing their Screen subclasses. See for example https://github.com/phetsims/calculus-grapher/issues/117.
(3) Where developer are parameterizing their Screen subclass, the model M does not implement TModel, but tsc does not complain about this error. See for example greenhouse-effect.PhotonsScreen, where
PhotonsModel
is not a modelclass PhotonsScreen extends Screen<PhotonsModel, PhotonsScreenView>
@samreid why do you think this is an appropriate use of IntentionalAny?