sun/buttons TypeScript review

[samreid]

I wanted to ask about some parts of the Sun TypeScript port:

• [x] Can we remove JSDoc that provides no information, such as:

// BooleanRectangularToggleButton.ts
  /**
   * @param trueNode
   * @param falseNode
   * @param booleanProperty
   * @param providedOptions
   */
  constructor( trueNode: Node, falseNode: Node, booleanProperty: IProperty<boolean>,
               providedOptions?: BooleanRoundToggleButtonOptions ) {

@pixelzoom EDIT: I removed a total of 2 cases of this, added param doc for 1 case. Not much of a real problem, and not a good use of my time.

• [x] I noticed some places use public and some places do not use public. We agreed this would be developer discretion, but it seems to be consistent within a file and within a repo:

// ButtonModel.ts
  public override dispose(): void {
    this.disposeButtonModel();
    super.dispose();
  }

  /**
   * Creates a PressListener that will handle changes to ButtonModel when the associated button Node is pressed.
   * The client is responsible for adding this PressListener to the associated button Node.
   */
  createPressListener( options?: PressListenerOptions ): PressListener {

@pixelzoom EDIT: @samreid has identified the only class (ButtonModel) that didn't use public, so I changed it to be consistent with the rest of sun/buttons/.

• [x] In FlatAppearanceStrategy, the type declaration for dispose: () => void; is after the constructor, but it would be preferable to move this before the constructor. Same in RectangularButton.ThreeDAppearanceStrategy.

@pixelzoom EDIT: There were bigger problems with dispose for AppearanceStrategy, fixed in https://github.com/phetsims/sun/commit/a59cef2c611135cabcf8197a803b48e35e11d4b5.

• [x] Some methods are missing return types, this occurs in multiple files. For example:

// PushButtonModel.ts

  public override dispose(): void {
    this.disposePushButtonModel();
    super.dispose();
  }

  /**
   * Adds a listener. If already a listener, this is a no-op.
   * @param listener - function called when the button is pressed, no args
   */
  addListener( listener: PushButtonListener ) {
    this.firedEmitter.addListener( listener );
  }

  /**
   * Removes a listener. If not a listener, this is a no-op.
   */
  removeListener( listener: PushButtonListener ) {
    this.firedEmitter.removeListener( listener );
  }

  /**
   * Fires all listeners.
   * (phet-io, a11y)
   */
  fire() {

    // Make sure the button is not already firing, see https://github.com/phetsims/energy-skate-park-basics/issues/380
    assert && assert( !this.isFiringProperty.value, 'Cannot fire when already firing' );
    this.isFiringProperty.value = true;
    this.firedEmitter.emit();
    this.isFiringProperty.value = false;
  }

@pixelzoom EDIT: I identified and fixed a total of 5 cases (including the 3 above). The return types were all void. Again, not much of a real problem, and not a good use of my time.

• [ ] Some assertions can be changed to type checks. For example:

// RectangularRadioButtonGroup.ts
    assert && assert( !options.hasOwnProperty( 'children' ), 'Cannot pass in children to a RectangularRadioButtonGroup, ' +
                                                             'create siblings in the parent node instead' );

    // make sure every object in the content array has properties 'node' and 'value'
    assert && assert( _.every( items, item => {
      return item.hasOwnProperty( 'node' ) && item.hasOwnProperty( 'value' );
    } ), 'contentArray must be an array of objects with properties "node" and "value"' );

• [ ] There are still some occurrences of visibility annotations:

  // RectangularToggleButton.ts
  // @private
  this.disposeRectangularToggleButton = () => {
    this.buttonModel.produceSoundEmitter.removeListener( playSounds );
    toggleButtonModel.dispose();
  };

• [x] This problem predates TS but I thought I'd mention it while in the area. new options.contentAppearanceStrategy( looks odd. Can we use a factory pattern instead of constructor pattern? This pattern looks a little unusual to me (or maybe I'm just not used to it)

  // TButtonAppearanceStrategy.ts
  type TButtonAppearanceStrategy = {
  dispose?: () => void;
  new( content: PaintableNode,
     interactionStateProperty: IProperty<ButtonInteractionState | RadioButtonInteractionState>,
     baseColorProperty: IProperty<Color>,
     options?: any ): any;
  }

pixelzoom EDIT: Moved to https://github.com/phetsims/sun/issues/752.

[pixelzoom]

I asked about code reviews at the most recent developer meeting and was told to wait until TypeScript conventions had solidified. So I'm surprised to see this issue, doing what I recommended, but the opposite of what @kathy-phet instructed.

I converted large pieces of sun/buttons/, but was not the only developer involved. Afaik, converting to TypeScript doesn't make me the owner. So perhaps this issue would be more appropriate to assign to @jbphet, the repo owner.

Can we remove JSDoc that provides no information, such as:

Our consensus was to add JSDoc if one or more fields are needed. We didn't discuss deleting existing JSdoc (some of which imo should have been fleshed out, but the original developer didn't do so). If this really bothers you, feel free to remove it.

I noticed some places use public and some places do not use public. We agreed this would be developer discretion, but it seems to be consistent within a file and within a repo:

I tried to stick with what had been done in the .ts file, but didn't always succeed. Sticking with the spirit of the file is (in general) a good convention and considered "polite" when working in a team.

I don't agree with a per-repo convention. That would be annoying and error-prone to follow, and we might as well have a project-wide convention.

If it were me, I'd require the public visibility modifier. The past and future discussion about this has already burned more time than some devs will claim to have "saved" by omitting public.

Again, if this bothers you, feel free to change it.

In FlatAppearanceStrategy, the type declaration for dispose: () => void; is after the constructor,

I've seen this general problem in other files. Apparantly not all developers are following the convention that fields precede the constructor. In this specific case, @jonathanolson converted ButtonNode FlatAppearanceStrategy. Feel free to move it.

Some methods are missing return types, this occurs in multiple files. For example:

I've been doing my best to add return types, and I can't speak to what other devs have been doing. Without tool support, it's an expensive way to require return types.

Looking through the first few files of cck-common in js/model/, I see an almost complete lack of return types.

This should be enforced project-wide by lint.

There are still some occurrences of visibility annotations:

I've been trying to remove these as I create fields and convert methods, again difficult to be 100% without tool support. Some developers appear to be consistently leaving these in. For example, createObservableArray.ts was converted by @samreid and contains 25 occurrences of @public, 4 of @private.

Some assertions can be changed to type checks.

Those type checks will only work for TypeScript sims. So I thought we had decided to leave assertions in for the benefit of the many JavaScript sims that we need to support.

This problem predates TS but I thought I'd mention it while in the area.

We decided to be mindful about making signficant code changes that are unreleated to TypeScript conversion, and to make those separately. This is unrelated to TypeScript and should be addressed separately. Please create a new issue.

[samreid]

Sounds good, thanks! Let's categorize this issue as part of the "deciding on conventions" developer meeting discussion.

[pixelzoom]

I noticed some places use public and some places do not use public. ...

If your goal is consistency, lint can enforce use of public, see explicit-member-accessibility. There is no tooling to prohibit use of public, though you could write a custom lint rule. And I guess you could allow devs to set the policy for repos that they're responsible for - which would be totally annoying for common code.

[samreid]

If your goal is consistency, lint can enforce use of public, see explicit-member-accessibility. There is no tooling to prohibit use of public,

The options for that rule are:

type AccessibilityLevel =
  | 'explicit' // require an accessor (including public)
  | 'no-public' // don't require public
  | 'off'; // don't check

Later on, the documentation describes that no-public forbids usage of the public modifier (instead of just not requiring it).

[kathy-phet]

Just a reminder on what was discussed / decided last meeting. Yes, code review is needed and should be prioritized, and also decided to do it informed by some agreed upon conventions.

From the notes of Thursday's meeting .... For the week of April 14 dev meeting, after we discussed some of these typescript dev meeting issues, can we all have a week when we review some else’s code.

So plan was for conversion discussion 4/14, then everyone to focus their typescript time on review that week of 4/14-4/21.

[pixelzoom]

I cleaned up sun/buttons/ and added annotations (see @pixelzoom EDIT) to https://github.com/phetsims/sun/issues/751#issue-1198575339. What I found was that sun/buttons/ was in very good shape, with very few instances of the problems that @samreid reported. I fixed a handful of inconsistencies in the above commits, and spent a lot of time locating them -- overall not a good use of my time.

While I agree that some of the conventions mentioned should be more generally discussed, I think review would be better focused on other repositories.

[pixelzoom]

6/9/2022 dev meeting:

@samreid will review the points that he brought up in https://github.com/phetsims/sun/issues/751#issue-1198575339. If there's anything that hasn't been generally addressed, he'll create an issue.

[samreid]

I believe all items identified above have been discussed. Closing.