Notes on bringing TS code up to the current standards

[pixelzoom]

As part of the TS sprint...

@kathy-phet would like me to take a making notes (a prioritized list of things to consider?) when bringing TS code up to the current standards.

[pixelzoom]

I'm not going to create a new doc, because this list is specific to this sprint (July 14-20, 2022), and it mostly references other existing docs.

For this TS sprint, here's what I feel are the priorities for bringing your code up to current standards:

(1) Clean up type aliases related to options, and usages of optionize and combineOptions, to conform to the examples in https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#options-typescript

(2) Document options properties (fields) where they appear in the type definition, not where their default values are assigned (typically in an argument to optionize).

(3) Document class properties (fields) where they are declared, not where they are instantiated. See https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md#documentation. (It's OK, and encouraged, to document the significance of a specific initial value where it's instantiated.)

(4) Prefer more-general types for APIs (fields, method parameters, return values) to hide implementation details and make the API more general - especially in common code! A couple of very general examples are shown below, and keep in mind that API design is all about context. NOTE: Most of the confusion that I see in APIs tends to be related to the "Property" hierachy, so ask questions in Slack#typescript.

/**
 * In this example, our API needs to expose the number of particles that are in our particle system, 
 * so that the value can be displayed in the UI.
 */
class ParticleSystem {

  // Too specific because callers do not need the additional functionality of NumberProperty.
  // For example, a caller might rely on numberOfParticlesProperty.rangeProperty, which is not 
  // something that we intended to expose.
  public readonly numberOfParticlesProperty: NumberProperty;

  // Preferred because it meets the requirements, does not expose unnecessary features of
  // how the Property is implemented, and makes it easy for us to change the implementation
  // in the future.
  public readonly numberOfParticlesProperty: IProperty<number>;

  constructor( ... ) {
    ...
    this.numberOfParticlesProperty = new NumberProperty( 0, {
      numberType: 'Integer',
      range: new Range( 0, 100000 ),
      tandem: options.tandem.createTandem( 'numberOfParticlesProperty' )
    } );
  } 
}

/**
 * In this example, MeterNode has an associated icon. The appearance of the icon is intended
 * to be immutable (cannot be changed) and used to label checkboxes, buttons, etc.  The icon
 * can be transformed (translated, scaled, etc.)
 */
class MeterNode {

  // Too specific because it exposes features of Path that are unnecessary for the requirements, 
  // could be used/abused by callers, and limit our ability to change the implementation in the future. 
  // For example, a caller could totally change the icon's appearance by setting icon.shape.
  public readonly icon: Path;

  // Preferred because it meets the requirements, does not expose unnecessary features of
  // how the icon is implemented, and makes it easy for us to change the implementation in the future.
  public readonly icon: Node;

  constructor( ... ) {
    this.icon = new Path( ... );
  }
}

(5) Convert deprecated enumerations to EnumerationValue or a string union type. Use EnumerationValue for rich enumerations. Use EnumerationValue or a string union type for non-rich enumerations, up to developer preference. (I prefer string union types.) More at https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md#enumerations.

[samreid]

Prefer more-general types for APIs (fields, method parameters, return values) to hide implementation details and make the API more general - especially in common code!

From today's meeting, we want to schedule a discussion around that. Perhaps related to articles like
https://enterprisecraftsmanship.com/posts/return-the-most-specific-type/
https://stackoverflow.com/questions/3434367/is-it-better-to-return-the-most-specific-or-most-general-type-from-an-action-met

UPDATE: I also saw this article: https://stackoverflow.com/questions/16934214/accept-the-weakest-return-the-strongest-but-why which says:

"return what's useful to the caller, and no more."

[pixelzoom]

7/19/2022 TS meeting:

We had a discussion about (4) in #1281 (comment). Summary:

• Regarding parameter types, I agree with the advice in Notes on bringing TS code up to the current standards #1281 (comment). Parameter types are informed by the needs of the method/function. If your method needs the features of Node, don't use define your param at Image. Prefer the most general type for parameters.

• Regarding return types, I do not agree with the advice in Notes on bringing TS code up to the current standards #1281 (comment). Blindly returning the type used in your implementation of a method/function is a bad practice. Return types should be informed by the needs of the API, and you should return the most general type that meets those needs.

• @kathy-phet asked me to add more comments to the examples in bullet 4, and that has been done.

[pixelzoom]

In Slack, @AgustinVallejo asked me about parameters.

Does it mean that, for all my methods, if a parameter doesn't use the features of its main type, should it be typed as the most general type it belongs to?

Parameters should be typed based on the needs of the implementation, not based on the type of the arguments that happened to be used when the method was implemented. Here's an example for sumProperties, a method that sums a set of Properties whose values are numbers.

// I have several Properties for the number of various types of particles.
// I plan to add more Properties like this in the future.
const numberOfElectronsProperty = new NumberProperty( 0, ... );
const numberOfProtonsProperty = new NumberProperty( 0, ... );
const numberOfNeutronsProperty = new NumberProperty( 0, ... );
const numberOfPhotonsProperty = new NumberProperty( 0, ... );

// I want to derive the total number of particles.
// This is where I discovered my need for a sumProperties methods.
const properties = [ numberOfElectronsProperty, numberOfProtonsProperty, numberOfNeutronsProperty, numberOfPhotonsProperty ];
const totalNumberOfParticlesProperty = new DerivedProperty( properties,
  () => sumProperties( properties ) );

// Here's my first implementation of sumProperties. Because I had NumberProperty instances
// above, I used NumberProperty[] as the type of my parameter.  This might be OK for a 
// single-use in a sim.  But as a general API, this implementation is unnecessarily restrictive.
// It requires the caller to use NumberProperty, but doesn't use any of the features of 
// NumberProperty, or even features of IProperty (like set) in the implementation.  Callers 
// will not be able to use instances of Property, TinyProperty, DerivedProperty, etc.
public sumProperties( properties: NumberProperty[] ): number {
  let sum = 0;
  properties.forEach( property => { sum += property.value; };
  return sum;
}

// This implementation is more flexible and general. The only feature used by the implementation
// is the abililty to get the value, so we type the parameter to meet that need.  In addition to 
// NumberProperty, this also allows the caller to provide an array whose elements can be 
// Property<number>, TinyProperty<number>, DerivedProperty, etc. -- or a mix of those types!
publiv sumProperties( properties: IReadOnlyProperty<number>[]: number {
  let sum = 0;
  properties.forEach( property => { sum += property.value; };
  return sum;
}

Questions?

[pixelzoom]

At 7/21/2022 dev meeting, @kathy-phet said she'd like to keep this issue open, so that information herein can be rolled into other documentation as appropriate (conventions, onboarding, etc.). Assigned to @AgustinVallejo to lead that effort.

[jbphet]

I've assigned myself to this, and will work with @AgustinVallejo to make sure that we have captured the information above in our TypeScript standards doc(s).

[marlitas]

@jbphet and @AgustinVallejo, looks like we added this to the onboarding documentation. Can this be wrapped up and closed now?