Should we remove type descriptions from comments when converting a file from JavaScript to TypeScript?

[jbphet]

Our current practice when writing JavaScript files is to include a fair amount of type description in our header comments. Here's an example for a method declaration from the file InteractionStrengthTable:

  /**
   * Get the interaction potential between two atoms.  Units are such that
   * the value divided by k-boltzmann is in Kelvin.  This is apparently how
   * it is generally done.  Note that this value is used as the "epsilon"
   * parameter in Lennard-Jones potential calculations.
   * @public
   * @param {AtomType} atomType1
   * @param {AtomType} atomType2
   * @returns {number}
   */

Here is an example for a field declaration, taken from MultipleParticleModel.js:

    // @public (read-only) {ObservableArrayDef.<ScaledAtom>} - array of scaled (i.e. non-normalized) atoms
    this.scaledAtoms = createObservableArray();

When a JavaScript file is converted to TypeScript, much of the information in these comments becomes explicitly declared in the source code. Should we remove it from the comments when such a conversion is made to avoid having redundant information that risks getting out of sync?

[jbphet]

I was curious as to whether removing a visibility annotation (e.g. @public) from a method in a TypeScript file would cause lint to fail, as it does for missing annotations in JavaScript files. It does, so this would need to be modified if we decide to move the annotations from comments.

[samreid]

In the commit, I made it so that TypeScript files are exempt from requiring method visibility annotations in the documentation, since they are provided in the language itself. A reminder that IDEs may need to be restarted after pulling to pick up this changed lint rule.

Should we remove it from the comments when such a conversion is made to avoid having redundant information that risks getting out of sync?

I agree with the recommendation in Effective TypeScript, Item 30 "Don't Repeat Type Information in Documentation", we should remove the redundant type information in comments. However, In the majority of my TypeScript conversions, I have left in the JSDoc type information in case we decide to abandon TypeScript. One exception is that I rewrote a significant part of circuit-construction-kit-common/js/model/analysis, and decided to omit the JSDoc for data-oriented types such as https://github.com/phetsims/circuit-construction-kit-common/blob/39ec47fec96ee7ffc74bc6afead0334ce2968143/js/model/analysis/DynamicCoreModel.ts, and I think this has generally been for the best.

As I get more experience with TypeScript, I'm becoming more confident that it will increase our efficiency, safety, sanity, etc. If we decide to abandon it, we will need to discuss what the .ts => .js migration would look like, and if we are OK having to re-introduce any JSDoc we delete now.

[chrisklus]

From 11/4/21 dev meeting:

JO: Yes, I think we should remove redundant information SR: yes, I agree that is our long term plan, but do we want to start doing that before we go all in on TS? JB: I'm okay with leaving it for now with an issue to go back through and remove everything. It sounds like we're on board with doing so once we've committed.

Consensus: No type information in the JSDoc, and we will keep JSDocs only if they are providing additional information.

[chrisklus]

Assigning to @samreid to make commits on this issue for refactoring.

[chrisklus]

CM: Are we still using the @public modifier? How about @public (read-only)? The TS read-only prevents writing from the owner and client, not just the client like I often like to do.

Possible option: use a private Property for writing internally and make a public Property that is read-only for clients.

SR: It feels like we need a new issue for discussing read-only, we will continue investigating there.

[chrisklus]

As for using public, JG prefers being explicit about including public.

JO: I would prefer to avoid more verbosity and get used to the implicit behavior SR: I agree with this.

CM: It would have been nice if TS defaulted to private, instead of making everything public by default. JO: Is there an option for turning this on?

Do we want to enforce one pattern right now or dev preference?

Consensus: We will leave it up to developer preference.

MK: Does the public part apply to constructors? JO: Dev preference is fine with me. SR: Same arguments apply, seems like it should the same consensus (dev preference).

[samreid]

This is now prevented by the (bad-typescript-text) lint rule, closing.