TModel now requires all models to have a `step` method.

[pixelzoom]

In Sim.ts, screen.model.step is clearly optional:

      if ( screen.model.step && dt ) {
        screen.model.step( dt );
      }

But in TModel.ts (created in https://github.com/phetsims/joist/issues/783), step is now required:

type TModel = {
  step: ( dt: number ) => void;
};

And Screen requires a model to be of type TModel:

class Screen<M extends TModel = IntentionalAny, V extends ScreenView = ScreenView> extends PhetioObject {

So as I'm converting sims to TypeScript (e.g. Equality Explorer), I'm having to add a no-op step methods to my models that previously had no step method. It probably won't cause a performance problem, but it was unexpected. And it seems like a pretty major change, and bit of an odd assumption, that all models will need step. Was this intentional? Was the fact that (many?) models don't have step considered?

Assigning to @zepumph, since it looks like he made the commit to TModel.ts that made step required for all models.

[pixelzoom]

I've also discovered that Screen is not enforcing the requirement that the model be a TModel. In ph-scale, which is fully converted to TypeScript, I have:

export default class MySolutionScreen extends Screen {
...
    super(
      () => new MySolutionModel( {
        tandem: options.tandem.createTandem( 'model' )
      } ),
      model => new MySolutionScreenView( model, ModelViewTransform2.createIdentity(), {
        tandem: options.tandem.createTandem( 'view' )
      } ),
      options
    );

... where MySolutionModel does not implement TModel and does not have a step method.

[pixelzoom]

In the above commits, I added implements TModel to the models in all of my TS sims, and properly parameterized Screen (even though it doesn't complain if it's not parameterized propertly).

4 of 5 sims required the addition of an empty step method to a top-level model class:

• beers-law-lab BeersLawModel
• equality-explorer SolveItModel
• molecule-polarity RealMoleculesModel
• ph-scale MySolutionModel

So there's evidence that NOT having a step method is common for models.

[zepumph]

@samreid and I felt like TModel made Joist typescript code more maintainable, but we realized that an empty step function was less than ideal. We couldn't figure out another definition of a model type that Screens could be passed. @samreid, do you want to reevaluate our decision?

[samreid]

I tried a variety of ways to tell TypeScript: "it may or may not have a step function, but if it does, it must have this signature", and it doesn't seem to be possible (when using implements TModel). https://stackoverflow.com/questions/56085306/error-message-an-interface-can-only-extend-an-object-type-or-intersection-of-o. So having empty step functions seems a reasonable alternative to me. OK to close?

[pixelzoom]

I would have thought that making step optional would work, like this:

type TModel = {
  step?: ( dt: number ) => void;
};

But removing (for example) the empty step from BeersLawModel results in:

TS2559: Type 'BeersLawModel' has no properties in common with type 'TModel'.

It appears that TModel needs to have at least 1 required Property for step?: to work. For example:

type TModel = {
  anything: number;
  step?: ( dt: number ) => void;
};

I can then remove the empty step method from BeersLawModel, if I add some like this to BeersLawModel:

 public readonly anything = 4;

Very strange... So I'm not sure how to make TModel.step optional unless it has at least 1 other required property.

@samreid Thoughts?

[pixelzoom]

The reason I brought this up is that it feels wrong to require an empty method that will be called on every frame. Maybe it's insignificant, maybe it gets optimized out, ... I don't know.

[pixelzoom]

How about defining TModel like this? :

type TModel = {
  step?: ( dt: number ) => void;
  reset: () => void;
}

Most TModel subclasses do in fact have a reset method, typically called by the ResetAllButton listener. And in the cases where an empty reset needs to be added, I'd much rather add something that is called by a button listener, instead of something that is called by Sim on every frame.

[pixelzoom]

TModel.reset has been proposed previously.

In https://github.com/phetsims/joist/issues/783#issuecomment-1054488070, @zepumph said:

Maybe this get's a reset function too?

In https://github.com/phetsims/joist/issues/783#issuecomment-1188426857, @samreid replied:

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.

I don't think it needs to be called by joist to be part of the TModel API. It just needs to be something that TModel typically has. And the advantage is that step can be optional.

[samreid]

Good idea! Taking it one step further, how about making both optional? It seems to work nicely. And we have several demos that don't have a reset function (I see 19 type errors if reset is required).

type TModel = {
  step?: ( dt: number ) => void;
  reset?: () => void;
};

[pixelzoom]

With:

type TModel = {
  step?: ( dt: number ) => void;
  reset?: () => void;
};

If I remove both step and reset from BeersLawModel (for example), then it errors with:

TS2559: Type 'BeersLawModel' has no properties in common with type 'TModel'

So if both methods are optional, at least one of them needs to be defined in the implementing class. Very weird, but I guess it makes sense for structural typing.

It may be less confusing to just make reset required, and add an empty reset in the few places where it's needed. And if we were willing tolerate adding many empty step methods, in performance-critical code, then I would think that it would be OK to add empty reset methods that will never be called in a few demo applications.

[pixelzoom]

@samreid said:

I see 19 type errors if reset is required

Here are the 19 errors:

Found 19 errors in 9 files.

Errors  Files
     1  ../../../bamboo/js/bamboo-main.ts:27
     1  ../../../joist/js/HomeScreen.ts:30
     1  ../../../joist/js/joist-main.ts:35
     1  ../../../mobius/js/mobius-main.ts:35
     1  ../../../nitroglycerin/js/nitroglycerin-main.ts:28
     6  ../../../scenery-phet/js/scenery-phet-main.ts:57
     4  ../../../sun/js/sun-main.ts:45
     1  ../../../tambo/js/tambo-main.ts:116
     3  ../../../vegas/js/vegas-main.ts:40

Every one of them currently has an empty step method. It seems like an empty reset method would be a fine tradeoff. (And it's really only 9 sites that need to be changed.)

[samreid]

Sounds good, I committed TModel as prescribed in https://github.com/phetsims/joist/issues/861#issuecomment-1271104966 and updated usage sites. Ready for review.

[pixelzoom]

Looks good, thanks for taking the lead on making the changes.

I identified a few more empty step methods to delete. See the above commits.

I also added a link to this issue in TModel.ts, in case we need to remember/revisit what has been done.

Closing.