Convert to TypeScript

[pixelzoom]

Based on my experiences with adding dynamic-locale support to other sims, it's easier if the sim is in TypeScript. For natural-selection (JavaScript), I did not convert to TypeScript, and it took ~2x as long as converting geometric-optics (a similar-size TypeScript sim). For molecule-polarity, I waited until I ran into problems to convert it to TypeScript, then the last (and most difficult) 20% went quickly. So...

Since ph-scale is a relatively small sim (5910 lines), I'm going to convert it to TypeScript before I do more work on #239 (add support for dynamic locale). I anticipate that this will make #239 easier/lower-cost, while getting TypeScript conversion for (almost) free. And having the sim in TypeScript will certainly be a win for future maintenance.

@kathy-phet FYI.

[kathy-phet]

Sounds good!

Sent from my iPhone

On Sep 3, 2022, at 3:44 PM, Chris Malley @.***> wrote:

 Based on my experiences with adding dynamic-locale support to other sims, it's easier if the sim is in TypeScript. For natural-selection (JavaScript), I did not convert to TypeScript, and it took ~2x as long as converting geometric-optics (a similar-size TypeScript sim). For molecule-polarity, I waited until I ran into problems to convert it to TypeScript, then the last (and most difficult) 20% went quickly. So...

Since ph-scale is a relatively small sim (5910 lines), I'm going to convert it to TypeScript before I do more work on #239 (add support for dynamic locale). I anticipate that this will make #239 easier/lower-cost, while getting TypeScript conversion for (almost) free. And having the sim in TypeScript will certainly be a win for future maintenance.

@kathy-phet FYI.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[pixelzoom]

All files are converted to TypeScript.

There are 24 type-related problems that need to be resolved. Search for this comment in code:

TODO https://github.com/phetsims/ph-scale/issues/242

[pixelzoom]

There are 9 TODOs remaining. They are all related to the fact that pH (and concentration) can be number | null. They are null when the beaker is empty.

[pixelzoom]

Done, closing.

[samreid]

I'm seeing one lint error, I'll reopen, fix the lint error and reassign for review.

The lint error is:

ph-scale: 
/Users/samreid/apache-document-root/main/ph-scale/js/macro/view/MacroScreenView.ts
  103:21  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion

[samreid]

Fixed and ready for review. Not sure why the lint rule triggered here but not on your side.

[pixelzoom]

Thanks for the fix. A more complete fix is in the commit above -- the local variable and assertion are no longer needed, because model.pHMeter is now pHMeter: MacroPHMeter instead of pHMeter: MacroPHMeter | null.

No idea why this didn't error in my local lint, or in git hooks.

Closing.

[samreid]

I think ph-scale and basics need to be added to tsconfig/all, I'll self-assign to do that.

[pixelzoom]

This is news to me. What is "tsconfig/all"? And when do I need to add a repo to it?

[pixelzoom]

Looks like you mean chipper/tsconfig/all/tsconfig.json. I have not been adding anything to this file, because I didn't know it was a requirement. Missing are: molecule-polarity, ph-scale, ph-scale-basics, beers-law-lab, concentration.

[samreid]

I added the missing remaining sims. After changes in https://github.com/phetsims/chipper/issues/1324, the linter will tell us if it trying to lint any files that are not covered by the tsconfig, so that will help. I think this issue can be closed.

[pixelzoom]

👍🏻