phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Missing `protected` modifiers in AccessibleNumberSpinner.ts and AcessibleValueHandler.ts #753

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

In AccessibleNumberSpinner.ts:

    // @protected - emits events when increment and decrement actions occur, but only for changes
    // of keyboardStep and shiftKeyboardStep (not pageKeyboardStep)
    readonly incrementDownEmitter: Emitter<[ boolean ]>; // @protected
    readonly decrementDownEmitter: Emitter<[ boolean ]>; // @protected

In AccessibleValueHandler.ts:

     * @protected
     */
    handleKeyUp( event: SceneryEvent<KeyboardEvent> ) {
...
     * @protected
     */
    handleChange( event: SceneryEvent ) {
...
     * @protected
     */
    handleInput( event: SceneryEvent<Event> ) {

If these are truly protected, then the TypeScript protected modifier should be used, and the JSDoc @protected annotations should be deleted.

The methods also need return types.

zepumph commented 2 years ago

Yes. Ideally that would be the case, just like all items prefixed with _ would be marked as private with typescript, but this cannot be done due to a limitation of how Typescript mixins work. If you do that, you get an error in which anonymous classes cannot have private or protected members.

pixelzoom commented 2 years ago

Well that certainly is unfortunate. And I now see the comment in the code:

  // Unfortunately, nothing can be private or protected in this class, see https://github.com/phetsims/scenery/issues/1340#issuecomment-1020692592

I've improved that comment to be a little more informative/correct, and not refer to the trait as "this class":

  // Unfortunately, private and protected modifiers cannot be used in this trait, due to a limitation of how Typescript 
  // mixins/traits work. If you do that, you get an error in which anonymous classes cannot have private or protected 
  // members. See https://github.com/phetsims/scenery/issues/1340#issuecomment-1020692592

Closing.