PhET TypeScript conventions for export are being violated.

[pixelzoom]

I first encountered this in VerticalCheckboxGroup.ts

export type VerticalCheckboxGroupOptions = VerticalCheckboxGroupSelfOptions & Omit<VBoxOptions, 'children'>;

… and I see this export type pattern using in many repos.

From https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md#multiple-exports:

When exporting multiple modules, all types should be combined into 1 export statement, and all non-types should be combined into 1 export statement. So a .ts file should have at most 2 export statements.

A possibly-incomplete list of repos where this needs to be fixed:

• axon
• build-a-nucleus
• center-and-spread
• counting-common
• joist
• number-play
• sun
• vegas

Assigning to @samreid and @chrisklus, since they seem to be responsible for many of these.

[samreid]

Recently @chrisklus suggested exporting option types inline and we have been exercising this strategy in Center and Spread to see how it goes. It has been working very seamlessly, and it seems like the best way to export options types. I recommend revising the conventions to promote this.

[pixelzoom]

It has been working very seamlessly, and it seems like the best way to export options types.

The best way to export types for you perhaps. The best way to export types for a team is to follow the conventions that we established. And it's fine to revisit conventions. But it would be more polite to have that discussion before propagating to common code. What you've done is more than "exercising this strategy in Center and Spread to see how it goes".

Here's how this wasted 1/2 hour of my time: WebStorm wasn't finding a type that I expected in VerticalCheckboxGroup.ts, so I went there to see if it was exported. It wasn't in the exports at the bottom of the file (where they're supposed to be by PhET's convention) so I added the type. Then I got some brand new TypeScript errors. A few minutes of head-scratching later, and I figured it out. Then I spent more time investigating, then creating this issue.

I get it that it may be easier for you to type export at the definition site. But that doesn't necessarily make it easier for others. We've discussed this convention before, I guess we can spend more time discussing it again.

[chrisklus]

When exporting multiple modules, all types should be combined into 1 export statement, and all non-types should be combined into 1 export statement. So a .ts file should have at most 2 export statements.

Inline exports for types don't break this convention unless multiple type are being exported, which I've never needed to do. I wasn't aware that grouping type and modules exports was required, and that was never specified, only shown in the example.

[samreid]

Today's dev meeting:

@pixelzoom does this mean we can/should also: export default class DotPlotNode extends Node {?
@jonathanolson: Note that scenery and other repos have an imports file.
@pixelzoom: Either place seems reasonable as long as we have a convention. IIRC we put them at the bottom so it was easy to see what is exported. I don't see a good reason to change the convention we have had so far.
@chrisklus: I was unaware of the convention to put it at the end of the file. I think it's convenient to export at the usage site but I'll defer to the team decision.

[samreid]

Result of the vote:

MK: First opinion is I don’t care, second opinion is to be able to export wherever you want. It is annoying to have to export a type at the bottom of the file. I see value in that annoyance, as it is meant to be an extra step to ensure that the API for a file is explicit and purposeful.
SR: Exporting at the declaration site seems nice. Not a really strong preference.
MG
CM I don’t see a reason to change the convention, but don’t want to spend time discussing it.
JG: Don’t really care, but it would be best to be consistent everywhere.
CK: slight preference for inline, no strong feelings
JO: Prefer exports at declaration, fine with either. Having to navigate to the bottom of the file a lot is a cost, I have to add exports way more than there would be a cost to me of finding them

Therefore it seems reasonable to be "developer preference", put them where you desire but be aware they may not always be at the bottom of the file. I'll update the documentation.

[samreid]

Fixed and ready for review, @pixelzoom would you like to review?

[pixelzoom]

👍🏻

[zepumph]

I don't think this is complete, I see https://github.com/phetsims/phet-info/blob/master/doc/best-practices-for-modules.md, which is pointed to in the CRC, can we please update those too. Or at least upvote my sentiment to combine conventions documents in #186 (comment).

[samreid]

I upvoted the comment #186 (comment) to combine conventions documents.

Also, the code review checklist now points to the typescript conventions doc, where this pattern is described. New development is in TypeScript, so perhaps this is sufficient. OK to close?