phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

Export and import IOTypes so usages look like PropertyIO instead of Property.PropertyIO #285

Closed samreid closed 1 year ago

samreid commented 1 year ago

When we designed the IO Type system, we did not utilize the fact that a file could have multiple exports. But it seems like a lot of usage sites would be easier to read and write if IO types could be written like Vector2IO or PropertyIO instead of Vector2.Vector2IO and Property.PropertyIO.

Tagging @pixelzoom and @zepumph so they aware this is coming up.

pixelzoom commented 1 year ago

Usages like Vector2.Vector2IO don't bother me. And in fact I think it's preferrable (and necessary) if you want to save/restore private parts of the core type. So if you're planning to change that, let's chat first.

pixelzoom commented 1 year ago

Oh... Maybe you're saying that Vector2.ts should simply add an export for Vector2.Vector2IO?

samreid commented 1 year ago

Yes, adding an export. I'll add a commit momentarily demonstrating it for Property.PropertyIO.

samreid commented 1 year ago

The commits show the proposal for exporting PropertyIO. @pixelzoom does that look good to you?

pixelzoom commented 1 year ago

I'm not sure that PropertyIO was a good choice as an example. Or maybe it was a good choice, to demonstrate problems with this proposal.

(1) PropertyIO is not defined in Property.ts, and is in fact associated with core type ReadOnlyProperty. So it violates the PhET naming conventions, and was therefore confusing. If conventions had been followed, I should have been looking at replacements of Property.PropertyIO with PropertyIO.

(2) Most IO Types I've seen are declared like public static readonly SomethingIO =. PropertyIO is declared as public static PropertyIO<T, StateType>( parameterType: IOType<T, StateType> ): IOType {...}. Maybe that's responsible for (3).

(3) The additional export requires an eslint-disable. Will that be needed for all IOType exports? Why is it needed for this one?

export const PropertyIO = ReadOnlyProperty.PropertyIO; // eslint-disable-line bad-text

That said...

Is this the general convention that you're proposing?

// definition
export default class MyType {
   public static readonly MyTypeIO = ...;
}

export const MyTypeIO = MyType.MyTypeIO;

// usage - either, at developer discretion
phetioValueType: MyTypeIO
phetioValueType: MyType.MyTypeIO

If so...

So my vote for this is proposal is "no".

samreid commented 1 year ago

I agree, having IO types be declared as a public static readonly property of the core type seems like the most important part, and it is not necessary to provide a shorthand for usage sites. Most commits will auto-revert, but I'll need to manually revert axon and energy-forms-and-changes. Coming up soon...

samreid commented 1 year ago

Thanks, I reverted the changes for this issue and elaborated on a new side issue https://github.com/phetsims/axon/issues/429. @pixelzoom can this issue be closed?

pixelzoom commented 1 year ago

👍🏻 closing.