Convert to typescript

[zepumph]

Wahoo!

[zepumph]

Please note https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md#L19. And we should probably go through the options and enums patterns together.

[zepumph]

After discussing with @matthew-blackman, I decided to convert a single model/view pair as reference. Here are some notes about my conversion:

• Note how I "widen" the public api of Properties whenever possible to the most general item. I only need to listen to the zoomProperty so it is a TReadOnlyProperty. positionProperty is a Property instead of TProperty because it needs a reset method.
• phetioDocumentation passed to DragListener wasn't doing anything (yay typescript for the bug fix)
• getJustDataProbeBounds didn't have the return type annotated.
• Take a look at the optionize pattern in DataProbeNode, that uses the ParentOptions pattern we discussed all to briefly this morning.

I'm excited to discuss further!

[matthew-blackman]

The common model/view is fully converted, so I think this may be a good time for a code review before the final conversion push @zepumph

[zepumph]

Overall things are looking really really good. Thanks for all the hard work here, it is really helping me to understand the sim code.

• Vector2Property is an implementation detail, and I recommend using a more general type that isn't so closely tied to a specific class for the Type:

  projectile-motion/js/common/model/ProjectileMotionMeasuringTape.ts

  Lines 19 to 20 in 81af58c

  	public readonly basePositionProperty: Vector2Property;
  	public readonly tipPositionProperty: Vector2Property;

• projectile-motion/js/common/model/ProjectileMotionModel.ts

  Lines 88 to 93 in 64e226c

  	* {ProjectileObjectType} defaultProjectileObjectType - default object type for the model
  	* {boolean} defaultAirResistanceOn - default air resistance on value
  	* {ProjectileObjectType[]} possibleObjectTypes - a list of the possible ProjectileObjectTypes for the model
  	* {Tandem} tandem
  	* options
  	*/

  and

  projectile-motion/js/common/model/ProjectileMotionModel.ts

  Line 375 in b1d51f4

  * {number} numProjectiles - the number of simultaneous projectiles to fire

  and

  projectile-motion/js/common/model/ProjectileMotionModel.ts

  Line 412 in b1d51f4

  * {ProjectileObjectType} selectedProjectileObjectType - contains information such as mass, diameter, etc.

  and probably others still has jsdoc type info in it and shouldn't. Perhaps a search for * { will help find the rest.
• Most likely all usages of providedOptions: should be providedOptions?:, but I'll let you confirm for each case.

• projectile-motion/js/common/model/Target.ts

  Line 21 in 7b28fdd

  public readonly positionProperty: NumberProperty;

  doesn't need to be a NumberProperty as I don't see range used anywhere.
• TrajectoryOptions duplicates parent options, and should instead use an empty selfOptions and call upon PhetioObjectOptions
• why is this a templated var that resolves a multiline single quote string?

  projectile-motion/js/common/model/Trajectory.ts

  Lines 138 to 142 in ad32d94

  	phetioDocumentation: `${
  	'The count of how old this projectile trajectory is. Older trajectories have more ' +
  	'opacity until they are subsequently removed. The most recent trajectory fired has rank 0. ' +
  	'The second most recent has rank 1.'
  	}`,

• Usages of merge should instead by combineOptions to keep type safety.
• BackgroundNodeOptions defines pickable, but that is a NodeOption, so we can use EmptySelfOptions here.

• projectile-motion/js/common/view/BackgroundNode.ts

  Line 37 in 64e226c

  private flatirons: Image;

  this could be defined as a Node, because it is only used as such, that makes for a more stable API if in the future you wanted to change that from an Image to something else.
• There are many many spots where readonly is not used and should be. For example

  projectile-motion/js/common/view/BackgroundNode.ts

  Lines 33 to 37 in 64e226c

  	private sky: Rectangle;
  	private grass: Rectangle;
  	private road: Rectangle;
  	private roadDashedLine: Line;
  	private flatirons: Image;

  .
• CannonNodeOptions defines tandem and renderer which are both parent options, let's not put those in SelfOptions also.
• FireButton.baseColor is a parent option.
• Something that may be good to try is to batch rename public to private in the whole repo and see how many errors you get. That may help find spots where you missed a private thing. Note though that it is possible to find something with no usages publicly that you still want to be public, but most often that happens in common code, and not sim-specific stuff.
• ProjectileMotionScreenViewOptions.tandem is a parent option
• These need to be marked as optional to get the benefits of optionize,

  projectile-motion/js/common/view/ProjectileMotionViewProperties.ts

  Lines 15 to 19 in d893d0e

  	type ProjectileMotionViewPropertiesOptions = {
  	tandem: Tandem;
  	forceProperties: boolean;
  	accelerationProperties: boolean;
  	};

• Note that ProjectileMotionViewProperties.tandem is NOT a parent option, good work defining it!
• ProjectileNodeOptions.preventFit is a parent option
• Note that even static members of ProjectileObjectViewFactory should be marked as readonly
• many options in ToolboxPanelOptions are from parent options
• We want to make a new issue to convert VectorsDisplayEnumeration to use the new Enumeration pattern
• I have already said this, but there are many spots where you have optional options with defaults, but the type doesn't have a question mark on it. That is not idea, even if all usages pass in a value for that, if there is a reasonable default, it should be marked as optional. Please look at this link and scan everywhere for the issue:

  projectile-motion/js/common/ProjectileMotionConstants.ts

  Lines 20 to 28 in af42e3b

  	export type ProjectileMotionUIOptions = {
  	controlsVerticalSpace: number;
  	xSpacing: number;
  	textDisplayWidth: number;
  	numberDisplayMaxWidth: number;
  	textDisplayHeight: number;
  	readoutXMargin: number;
  	sliderLabelSpacing: number;
  	} & PanelOptions;

• I think a couple more usages of satisfies would be nice in ProjectileMotionConstants when it is clearly an options block for a particular type's options.

UPDATE: 1/11/23

• Please see CT required tandems must be supplied #319 for some work in tightening up ScreenView classes, each should require a tandem, and so we should be thoughtful about that when converting to typescript. Let's make sure each of those options types require a tandem (using PickRequired if needed).

Thanks for all the good work. I definitely went fast and could have missed some stuff. I'd say stay diligent as you go through these items and feel free to convert the rest. I also think we likely want another issue to try to tease out the options type where we were mixing things in ProjectileMotionUIOptions. Likely a separate issue, and I believe you will run into that much much more in the specific screen code. Thanks!

[samreid]

@matthew-blackman has said the conversion to TypeScript is partway done, and I see lots of great review recommendations above. Also, this sim is in a sprint team for this sprint, so I think will get appropriate attention. Thus, removing the ready-for-review label for now.