Lack of accessibility modifiers in mixins/traits

[pixelzoom]

In mixins/traits, I see comments like this one in NumberSpinner.ts:

  // 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

But when I use private and protected, I’m not seeing any tsc or lint errors. Was this perhaps a problem in an earlier version of tsc that has been fixed? Can we now use proper modifiers in mixins/traits?

[pixelzoom]

Slack#typescript:

@samreid
I’m seeing the same thing. I changed

    /* private */
    public _disposeAccessibleSlider: () => void;

to be private and the type checker still passed.

@pixelzoom
I’ve added correct accessibility modifiers for many mixins/traits, and I’m seeing no tsc or lint errors. And as a bonus, WebStorm shows me that a large number of the private fields should be readonly.

@pixelzoom
@zepumph @jonathanolson since you’re the main creators of mixins/traits, is there any reason why I should not commit this?

@pixelzoom
Hmm…. Now I am seeing some errors, in scenerySerialize.ts. I’m going to bail. Too bad.

@pixelzoom
FYI, there are many fields in mixin/traits that should be readonly, and quite a few documented “private” (with underscore name prefix) that are in fact public or protected. So this pattern is definitely not a good fit for type-checking.

[pixelzoom]

This is really not a good fit for our decision in #1258 to require accessibility modifiers. I've had to this nonsense for now, e.g. in AccessibleValueHandler, adding a public modifier and then documenting as `/* private */:

    /* private */
    public _valueProperty: IProperty<number>;
...
    /* private */
    public _updateAriaValueText( oldPropertyValue: number | null ): void {

And as I mentioned above:

• Many fields in mixin/traits that should be private readonly, and the need to make them public is obscuring the readonly
• Quite a few documented “private” (with underscore name prefix) that are in fact public or protected. So the underscore prefix isn't serving us well.

[zepumph]

How about turning off that rule for the whole file. We are already doing this for @protected

Index: js/accessibility/voicing/Voicing.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/voicing/Voicing.ts b/js/accessibility/voicing/Voicing.ts
--- a/js/accessibility/voicing/Voicing.ts	(revision ace23fc4566773387bb33efa9960b4248b702f5f)
+++ b/js/accessibility/voicing/Voicing.ts	(date 1654872769089)
@@ -26,6 +26,7 @@
 
 // Disable for the whole file
 /* eslint-disable no-protected-jsdoc */
+/* eslint-disable @typescript-eslint/explicit-member-accessibility */
 
 import inheritance from '../../../../phet-core/js/inheritance.js';
 import ResponsePacket, { ResolvedResponse, ResponsePacketOptions, VoicingResponse } from '../../../../utterance-queue/js/ResponsePacket.js';

Does this help with bullet one? readonly _somePrivateMember and we don't need public to be manually added to an actually private field?

Quite a few documented “private” (with underscore name prefix) that are in fact public or protected. So the underscore prefix isn't serving us well.

This is unfortunate. I couldn't think of another thing to do, so I think we should clean this up, and make underscore name prefix private. We know that some mixins will stick around for the foreseeable future, so let's make this convention right. Do you have another idea?

[pixelzoom]

I agree that not requiring public in mixins/traits might be preferrable. And maybe also use JSdoc annotations in mixins/traits, in lieu of accessibility modifiers.

readonly _somePrivateMember isn't going to help with identifying what should be readonly. WebStorm only assists with that if a field has the private modifier. I guess you could add private to all fields, have WebStorm tell you which ones should be readonly, then remove private -- but what a pain.

But before we do any of that... I'd be interested in knowing why use of private and protected is a problem for some PhET mixins/traits, and not for others. @jonathanolson @zepumph any insight on that?

[zepumph]

Thesis: I think we can have support for private and protected in our mixins.

We are currently running into this error because I create a mixin like so:

const VoicingClass = class extends InteractiveHighlighting( Type, optionsArgPosition ) {

This is an anonymous class, and typescript doesn't let them have private/protected members, explained and discovered with TypeScript issue pointers over in phetsims/scenery#1340 (comment). BUT, it seems like it is easy enough to name the class (stupid me). I'm not seeing any errors, so perhaps it was a flaw in my understanding. And for that I'm sorry for this headache! I'll commit changes to 6 mixins imminently.

[zepumph]

I feel dumb that we needed the above commit in the first place, but, getting over myself, it is really really nice to have this fixed! I added visibility annotations to the sun and scenery a11y feature mixins, and cleaned up the lint rules since we don't need to opt out of forbidding @protected anymore. @pixelzoom please review.

[pixelzoom]

Excellent, glad we can add accessibility modifiers to mixins. It makes me feel even better about our decisions in #1258.

Here's the full list of mixins/traits. I've checked off the ones that @zepumph and I have handled.

@jonathanolson is there any reason why we can't add proper accessibility modifiers to these?

mobius

• ThreeInstrumentable

scenery

• InteractiveHighlighting
• ReadingBlock
• Voicing
• HeightSizable
• Sizable
• WidthSizable
• FlowConfigurable
• GridConfigurable
• Imageable
• Leaf
• Paintable
• RichTextCleanable (in RichText)
• DelayedMutate

sun

• Popupable
• AccessibleNumberSpinner
• AccessibleSlider
• AccessibleValueHandler

wilder

• Mixable

[samreid]

It may be safest to develop with incremental: false while working on this due to problems with the type checker which may be related to mixins, please see microsoft/TypeScript#49527

[chrisklus]

From 6/16/22 dev meeting:

We checked in on this issue and it will continue getting worked on.

[zepumph]

Looks like @samreid's bug report in typescript was tagged as fixed. Not sure what release it will get into, but yay!

microsoft/TypeScript#49527

[samreid]

At today's meeting, @jonathanolson confirmed he will champion this issue. Unclear how long it will take. @zepumph says it will be a straightforward refactor.

@pixelzoom said he tried to work on this, but couldn't add accessibility modifiers since it was unclear what should be private/public/protected.

@jessegreenberg: Pre-existing underscores may indicate it should be private.
@zepumph: The underscores were unfortunately quite often wrong.

@jonathanolson hopes this is a quick issues.

[jonathanolson]

I hit all of the above mixins - a lot of the layout things need to be scenery-internal. Closing