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: https://github.com/phetsims/projectile-motion/blob/81af58c0b563edaf10c9d04423c661aaa6a415ac/js/common/model/ProjectileMotionMeasuringTape.ts#L19-L20
• [x] https://github.com/phetsims/projectile-motion/blob/64e226c72de599992284aad7ab44af4ac7d5d574/js/common/model/ProjectileMotionModel.ts#L88-L93 and https://github.com/phetsims/projectile-motion/blob/b1d51f4f9fece9d165c4d80b14487385430dcc48/js/common/model/ProjectileMotionModel.ts#L375 and https://github.com/phetsims/projectile-motion/blob/b1d51f4f9fece9d165c4d80b14487385430dcc48/js/common/model/ProjectileMotionModel.ts#L412 and probably others still has jsdoc type info in it and shouldn't. Perhaps a search for * { will help find the rest.
• [x] Most likely all usages of providedOptions: should be providedOptions?:, but I'll let you confirm for each case.
• [ ] https://github.com/phetsims/projectile-motion/blob/7b28fdd9514d1f932aae60dc7bec812edf71de86/js/common/model/Target.ts#L21 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? https://github.com/phetsims/projectile-motion/blob/ad32d9452521c765723bf1612964697814e7c033/js/common/model/Trajectory.ts#L138-L142
• [ ] 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.
• [ ] https://github.com/phetsims/projectile-motion/blob/64e226c72de599992284aad7ab44af4ac7d5d574/js/common/view/BackgroundNode.ts#L37 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 https://github.com/phetsims/projectile-motion/blob/64e226c72de599992284aad7ab44af4ac7d5d574/js/common/view/BackgroundNode.ts#L33-L37.
• [ ] 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, https://github.com/phetsims/projectile-motion/blob/d893d0edcd2daa728c774cef61ec4b092d5cfbdc/js/common/view/ProjectileMotionViewProperties.ts#L15-L19
• [ ] 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: https://github.com/phetsims/projectile-motion/blob/af42e3b03419ae42fe7ca650a620976727deb0f8/js/common/ProjectileMotionConstants.ts#L20-L28
• [ ] 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 https://github.com/phetsims/projectile-motion/issues/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.