explicit-member-accessibility typescript-eslint rule

[pixelzoom]

Here's an example of why I'd like to revisit the decision to make public optional (at developer discretion).

In bending-light GridCanvasNode.ts:

class GridCanvasNode extends CanvasNode {
  modelViewTransformProperty: Property<ModelViewTransform2>;
  strokeDash: number[];
  gridLines: ObservableArray<{ x1: number; y1: number; x2: number; y2: number; lineDashOffset: number }>;
...
  constructor( gridLines: ObservableArray<{ x1: number; y1: number; x2: number; y2: number; lineDashOffset: number }>,
               modelViewTransformProperty: Property<ModelViewTransform2>, strokeDash: number[], providedOptions?: NodeOptions ) {

    super( providedOptions );
    this.gridLines = gridLines; // @private
    this.modelViewTransformProperty = modelViewTransformProperty; // @private
    this.strokeDash = strokeDash; // @private
  }

The fields here were probably auto-created using WebStorm, with default public visibility. And then they were never given the proper visibility, which should be private (as noted in the comments).

And this is not an isolated case. While working on #1224 to remove redundant @private annotations, I'm finding this problem throughout the fields and methods of bending-light. And I've seen it elsewhere.

My feeling is that requiring developers to explicilty add an annotation is more likely to result in a correct annotation. And (so far) some developers have abused default visibility.

[pixelzoom]

I should mention that the readonly modifier is also missing from the field declarations in the example above. This is another common problem that I'm seeing throughout PhET code.

[pixelzoom]

And again in SeriesCanvasNode.ts:

class SeriesCanvasNode extends CanvasNode {
  seriesProperty: Property<DataPoint[]>;
  modelViewTransformProperty: Property<ModelViewTransform2>;
  color: string;
...
  constructor( seriesProperty: Property<DataPoint[]>, modelViewTransformProperty: Property<ModelViewTransform2>,
               color: string, providedOptions?: NodeOptions ) {

    super( providedOptions );
    this.seriesProperty = seriesProperty; // @private
    this.modelViewTransformProperty = modelViewTransformProperty; // @private
    this.color = color; // @private
  }

[samreid]

We discussed this at today's dev meeting. @zepumph recommend we separate this into 3 problems:

• public modifier. Lint rule that can require modifiers: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md
• readonly modifier
• Lint rule that requires readonly for private members where possible: https://typescript-eslint.io/rules/prefer-readonly/

[samreid]

Have you been mindful of public/private?
AV - Forgot it existed, using the default
CM - yes, and enforced in package.json
CK - yes, and i opted to use public explicitly in NP
JB - I generally try to specify public explicitly
JG -
JO - Don’t mind either way, yes mindful of that
KP
LM
LV - yes, I try to use public explicitly
MK- I’m sorry I haven’t been a good participant.
MP
MS: I’m being mindful.
SR: I’m being mindful of public vs private during new code development. Making things private where possible

Recommendation for going forward, for voting for requiring public annotation: #1258
AV - Sounds like explicit is better
CM - explicit
CK - prefer explicit
JB - yes for explicit use of the “public” specifier
JG - Prefer explicit (especially for methods).
JO - Don't mind either way
KP - Yes, explicit
LM
LV
MK - Require some sort of this rule, turning it on for properties (public too) sounds really great.
MP
MS: Generally leaning towards dev choice, but am open either way.
SR: Can we try turning on this rule for a week and see how annoying it is?
Vote tally:
Explicit: 7
Dev Choice or Don’t feel strongly: 3

[samreid]

Several team members recommended that we should be consistent, even requiring public|private|protected on constructors, methods, getters, etc.

[samreid]

@samreid will also check why --fix isn't working.

UPDATE: According to https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts, it appears the only support for autofix is removing an unwanted "public" modifier.

[samreid]

I added a (horribly inefficient) script that adds accessibility modifiers.

[samreid]

I used the script in axon to make all missing modifiers private, then I used the type checker to visit each site and make them public where appropriate.

[pixelzoom]

I'd be interested in a script like this for general use. For some collection of .ts files, the script would change all fields to private readonly and methods to private (which should be the default, if a language is really serious about creating a good API) regardless of how you had set them. Then let tsc identify where modifiers need to be changed. I'm not sure how much effort this would be, but it would certainly catch incorrect modifiers.

[samreid]

Here is the result of grunt lint-everything --chip-away for this rule:
5827 problems (5827 errors, 0 warnings)

• aqua: @zepumph 1 errors in 1 files.
• bamboo: @samreid 113 errors in 25 files. (@pixelzoom)
• bending-light: @samreid 374 errors in 62 files.
• center-and-variability: @samreid, @chrisklus 153 errors in 42 files.
• circuit-construction-kit-common: @samreid 612 errors in 111 files.
• gravity-and-orbits: @samreid 239 errors in 39 files.
• build-a-nucleus: @Luisav1, @chrisklus 37 errors in 15 files.
• buoyancy: @jonathanolson 4 errors in 4 files.
• counting-common: @chrisklus 14 errors in 8 files.
• density-buoyancy-common: @jonathanolson 662 errors in 70 files.
• density: @jonathanolson 3 errors in 3 files.
• dot: @jonathanolson 517 errors in 13 files.
• greenhouse-effect: @jbphet 92 errors in 18 files.
• joist: @zepumph 3 errors in 3 files.
• kite: @jonathanolson 438 errors in 14 files.
• mobius: @jonathanolson 65 errors in 10 files.
• my-solar-system: @jonathanolson 115 errors in 37 files.
• number-play: @chrisklus 65 errors in 48 files.
• phet-core: @jonathanolson 79 errors in 10 files.
• phet-io-wrappers: @zepumph 10 errors in 2 files.
• ratio-and-proportion: @zepumph 2 errors in 2 files.
• scenery: @jonathanolson 2096 errors in 94 files.
• shred: @jbphet 1 errors in 1 files.
• tambo: @jbphet 63 errors in 38 files.

[pixelzoom]

vegas, sun, scenery-phet, and simula-rasa are all cleaned up, and are checking explicit-member-accessibility via their package.json.

[pixelzoom]

This rule makes most of the Access Modifiers section irrelevant in PhET's TypeScript Conventions doc. Should we remove the entire section? Keep part of it?

[zepumph]

I would remove it! It is out of date anyways

[zepumph]

After that, feel free to close, I cleaned up all package.jsons that were using rule.

[pixelzoom]

@samreid do you have any feelings about blowing away the entire Access Modifiers section? I think that you may have been the author of the final 2 paragraphs.

[samreid]

Looks good to be deleted, thanks!

[samreid]

In today's meeting, we agreed this issue can be closed when:

• The section mentioned above is deleted

[pixelzoom]

Access Modifiers section deleted in the above commit. Closing.