Can PhetioDynamicElementContainer.disposeElement be marked as public?

[samreid]

From phetsims/circuit-construction-kit-common#749, even though the overrides of disposeElement are marked as public, TypeScript used the parent annotation. But I was confused about why it is protected in the base class? Should it not be called typically? I'll mark it as public to make progress in phetsims/circuit-construction-kit-common#749, but would be good to discuss. Self assigning to take a closer look, but I'll consult with @zepumph soon.

[samreid]

I also changed @public (phet-io) to just @public in PhetioCapsule. (Probably not related to that change), I can no longer find any type errors with marking PhetioDynamicElementContainer.disposeElement as @protected. @zepumph want to do anything more with this issue?

[zepumph]

TypeScript used the parent annotation.

In general, is this a problem that will be fixed when we convert tandem to Typescript?

RE Your commit:
This is fine, since the doc next to public describes how we don't actually want to call this directly (PhetioDynamicElementContainer doesn't actually keep a container, the subtype does, so disposing and element will almost always need to have a subtype dispose to remove the element from a data structure). It isn't ideal though. I think that the annotations as they were were correct and ideal as I thought about the relationship.

[samreid]

You are right and it would be better to have this marked as protected in the base class and public in the subclasses. However, I tried that again and confirmed that TypeScript doesn't understand it. It gives this error again:

../../../circuit-construction-kit-common/js/model/Circuit.ts:921:24 - error TS2445: Property 'disposeElement' is protected and only accessible within class 'PhetioDynamicElementContainer' and its subclasses.

921       this.vertexGroup.disposeElement( oldVertex );
                           ~~~~~~~~~~~~~~

Even though it knows vertexGroup is of type PhetioGroup and not just PhetioDynamicElementContainer. However, this may just be a flaw in TypeScript's JSDoc layer, since this code runs OK:

class A {
  // @protected
  protected getName() {return 'a';}
}

class B extends A {

  // @public
  public getName() {return 'b';}
}

const b = new B();
b.getName();

OK to close?

[samreid]

For the record, I also noticed this code doesn't type check:

class A {
  // @private
  private getName() {return 'a';}
}

class B extends A {

  // @public
  public getName() {return 'b';}
}

const b = new B();
b.getName();

js/model/Circuit.ts:70:7 - error TS2415: Class 'B' incorrectly extends base class 'A'.
  Property 'getName' is private in type 'A' but not in type 'B'.

70 class B extends A {
         ~

[zepumph]

Should we put a TODO to mark this as protected in the super class when converting it to Typescript?

[samreid]

I added a TODO and referenced back to this issue. Want to go on hold until then?

[zepumph]

This file is now in typescript, and disposeElement is marked as protected. yay. Closing