TypeScript review of a few classes

[pixelzoom]

At 4/14/2022 TypeScript meeting, I volunteered to review:

• [x] Imageable
• [x] Pool
• [x] DynamicProperty
• [x] Font
• [x] Text

[pixelzoom]

Text review:

• I like the export default class convention, I've been using that everywhere.
• Should initializePhetioObject be protected? It's annoated as @protected and overrides a protected method.
• Delete redundant param type expressions, like @param {Object} baseOptions
• I see that you've omitted return types when they are void. Is that intentional or a coincidence? Will that be problematic when a lint rule is turned on? Or should it be a PhET convention to omit void at developer descrection?
• Should merge be replaced with optionize? Why or why not?
• We decided to ditch @param config and use @param providedOptions throughout PhET code. I still see @param {Object} config.
• @returns {string} is redundant.
• Where should we define statics like static TextIO: IOType; ? I've been putting them before the constructor, with fields. Looks like you've been putting them at the end of class definitions. Does it matter?

[pixelzoom]

Font review:

• There are many fields with an underscore prefix, e.g. _style. They are presumably private, and should have a private modifier.
• Several redundant assertion expressions, like typeof this._style === 'string' that are now handled by the type checker. (WebStorm flags these.) What's recommended in general for conversion to .ts? Should we keep assertions like this for JavaScript sims?
• Address TODO: get merge working in typescript. This TODO should have an associated issue.

[pixelzoom]

DynamicProperty review:

• isExternallyChanging is problematic. It's public and writeable, and looks like should only be read by clients. Replace with private _ isExternallyChanging and an ES5 getter?
• defaultValue, propertyPropertyListener, propertyListener can all be readonly (WebStorm flags them as such)
• Mismatch between @param [options] - options and providedOptions?:, update JSDoc (WebStorm flags this)

[pixelzoom]

Imageable review:

• More assertions like typeof width === 'number' that are now handled by the type-checker. (Webstorm flags these.)

[pixelzoom]

Pool review:

• partialConstructor, DefaultConstructor, initialize, and useDefaultConstruction should all be readonly, according to WebStorm

[pixelzoom]

Review completed.

General questions related to PhET conventions (EDIT: I created 4 issues, one for each bullet):

• Is it OK to omit void return type, or should all methods and functions have a return type? If we omit void, does that complicate being able to confirm that there's a return type? (Tracking in https://github.com/phetsims/chipper/issues/1225.)

• When is it OK to continue to use merge vs converting to optionize? (Tracking in https://github.com/phetsims/chipper/issues/1226.)

• Should assertions that are now handled by the type-checker be kept or deleted? If we keep them, is there a WebStorm setting to keep them from being flagged (highlighted)? (Tracking in https://github.com/phetsims/chipper/issues/1227.)

• Where should static field definitions appear? Above constructor? End of class definition, after all methods? Don't care? My preference is for "above constructor", so that all fields are in one place, making it easier to see the API at a glance. (Tracking in https://github.com/phetsims/chipper/issues/1228.)

Over to @jonathanolson.

[pixelzoom]

• Is it OK to omit void return type, or should all methods and functions have a return type? If we omit void, does that complicate being able to confirm that there's a return type?

Verifying that methods and functions have a return type is done by compiler option noImplicitReturns. It's a boolean, so does not appear to support the option to omit void. So unless there's another way, I recommend enabling noImplicitReturns in tsconfig-core.json, then adding void return types where needed.