search

phetsims / phet-info

Collection of information shared by PhET team members for the purpose of using github effectively and for other process-related topics.
http://scenerystack.org/
MIT License
64 stars 27 forks source link

Document anti-patterns for TypeScript options #191

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

In https://github.com/phetsims/phet-info/issues/187 and https://github.com/phetsims/chipper/issues/1217, I identified some ways that PhET's TypeScript options pattern can be abused (anti-patterns). I was asked to document them in the TypeScript Conventions document. But I think it makes more sense to put them in the Design Patterns document, which is what I'll do.

pixelzoom commented 2 years ago

@zepumph this is ready for review.

I added a new section to the PhET Design Patterns document, see Options (TypeScript). The anti-patterns identified in #187 and https://github.com/phetsims/chipper/issues/1217 were reframed as "guidelines", and I added several other guidelines that are not identified in those issues.

This was not intended to be a complete description of this pattern -- it's a start. And the examples are terse. Significant familiarity with optionize etc. is needed to get the most out of the examples.

The document and WilderOptionsPatterns.ts refer to each other. Feel free to move information between them as you see fit.

Also note that the original section about options was renamed to Options and Config (JavaScript) and is still relevant since PhET will continue to have JavaScript code for some time.

zepumph commented 2 years ago

Everything looks really good. I think there is a reasonable separation between the type of info in WilderOptionsPatterns and this document. This comes across as best practices, whereas the examples in Wilder are either hello world cases (a bit of a precursor to the scope of the design pattern), or examples demonstrating known bugs/cases for maintaining/improving optionize itself.

(5) If a class has no parent class, pick a field from the type that defines that field, rather than duplicating that field’s definition.

I think this is meant to say if a class DOES have a parent class, but perhaps another few works are needed as well:

To take a single option from a parent class, pick a field from the type that defines that field, rather than duplicating that field’s definition.

Or perhaps I don't understand.

All else is excellent. Thanks

pixelzoom commented 2 years ago

Thanks for the review.

(5) is worded as intended, but my example had a copy-paste error, corrected in the above commit. Here it is again:

(5) If a class has no parent class, pick a field from the type that defines that field, rather than duplicating that field’s definition.

// Our parent class has no parent class.
class MyClass { ... }

// incorrect, definition of PhetioObjectOptions.tandem is duplicated
type SelfOptions = {
  tandem: Tandem;
  ...
};
type MyClassOptions = SelfOptions;

// correct, definition of tandem is picked from PhetioObjectOptions, where it is defined
type SelfOptions = { ... };
type MyClassOptions = SelfOptions & PickRequired<PhetioObjectOptions, 'tandem'>;

Does this make more sense now @zepumph ?

zepumph commented 2 years ago

Yes perfect. Thanks. Closing. Feel free to reopen.

pixelzoom commented 2 years ago

Reopening - I still need to do a PSA for the dev team.

pixelzoom commented 2 years ago

In Slack:typescript, I posted:

PSA: For https://github.com/phetsims/phet-info/issues/191, I’ve added a new section to the PhET Design Patterns doc, “Options (TypeScript)“, see https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#options-typescript. If you have any questions or suggestions, feel free to ask here or in the issue. And please review this section at your earliest convenience, so that https://github.com/phetsims/phet-info/issues/191 can be closed.

I'll leave this issue open for a week or so, to see if there are any questions.

pixelzoom commented 2 years ago

5/12/22 TypeScript meeting:

In the next week, please read https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#options-typescript. If you have questions or suggestions, please add comments to this issue, or ask in Slack:typescript. Feel free to edit the document directly to correct typos, or add additional guidelines/examples.

When you have read the document, check yourself off in this list, and unassign yourself from this issue.

jbphet commented 2 years ago

This is awesome, and I can tell I'll be referencing it a lot!

I made a few doc edits, feel free to review. Here is a reference for some of the changes I made.

I had a couple of thoughts while reading through this:

samreid commented 2 years ago

This reads very nicely and is helpful, thanks! 

type AtomizerOptions = {
  numberOfAtoms: number;
};

type MyAtomizerOptions = AtomizerOptions & PickOptional<AtomizerOptions, 'numberOfAtoms'>;

// Type error: numberOfAtoms is still required because Required & Optional = Required
const m: MyAtomizerOptions = {};

(5) If a class has no parent class, pick a field from the type that defines that field,

But it's unclear what the relationship is. Should this say "from a type that defines the field?" I don't think so, but it's unclear why MyClass would want to pick from PhetioObjectOptions in this example. Can you clarify?

AgustinVallejo commented 2 years ago

Thank's for the read, it's been really interesting :)

A few questions:

pixelzoom commented 2 years ago

Re @AgustinVallejo's feedback in https://github.com/phetsims/phet-info/issues/191#issuecomment-1126091850 ...

Examples (1), (4), and (8) did indeed have typos, corrected in the above commits. Thanks for pointing them out.

The fragment "we want to hide the parent class’s options" has been changed to "we want to exclude the parent class’s options from our API". Hopefully that makes more sense. Let me know if it doesn't.

pixelzoom commented 2 years ago

Re @jbphet's feedback in https://github.com/phetsims/phet-info/issues/191#issuecomment-1125531543 ...

We might want to make it clear that some of the utility types used in these examples, such as Omit, are built-in TypeScript things whereas others, such as PickRequired, were created by us.

Beyond the scope of this section imo. Developers should be familiar with both TS and PhET "utility types", and they are independent of options, and are generl programming APIs. But if you feel strongly, feel free to add it.

I did add links to PickRequired.ts and PickOptional.ts in the text.

  • I was wondering whether the set of utility types that are demonstrated here are believed to cover all the cases we will need, so that we should never end up using ones like Partial, Exclude or the raw Pick.

I've worked on a lot of common code and sim code, and (so far) I've only had a need for the utility types noted in the examples. If additional utiility types are found to be useful, I think the appropriate place to add them is in additional examples. For the same reasons as I gave above, I don't this it's appropriate to add a general tutorial about utility types to this section of the patterns document.

pixelzoom commented 2 years ago

Re @samreid's feedback in https://github.com/phetsims/phet-info/issues/191#issuecomment-1125632378.

  • [ ] I also noticed this part of Example 3 is incorrect: ...

Thanks, fixed in above commit.

  • [ ] We should discuss whether we want a stricter version of Omit. ...

This is being discussed in https://github.com/phetsims/scenery-phet/issues/730#issuecomment-1125629164. When that issue is resolved, feel free to modify examples to match the new reality. In the meantime, examples match the current reality.

  • [ ] For (5), it says:

(5) If a class has no parent class, pick a field from the type that defines that field, But it's unclear what the relationship is. Should this say "from a type that defines the field?" I don't think so, but it's unclear why MyClass would want to pick from PhetioObjectOptions in this example. Can you clarify?

In example (5), MyClass has no parent class. It needs a tandem option. Rather than create its own definition of the tandem option, it should get that definition from the type that defines tandem, which is PhetioObjectOptions. I've (hopefully) clarified the example in the above commit. Let me know if you still have questions.

They don't relate. The examples reflect the current reality, not proposals. If a proposal results in a need to update an example or guideline, anyone can (and should) do that.

marlitas commented 2 years ago

This really helped me understand optionize more. @chrisklus and I used this just last week, and though it took us down a rabbit hole we came out the other side much stronger for it. Thanks for writing this out. I didn't see anything additional to some of the typos mentioned above and saw they're now fixed.

Removing my assignment.

jessegreenberg commented 2 years ago

Thanks, the examples and documentation were understandable/easy to follow. Ill definitely continue to refer to this when working with options.

pixelzoom commented 2 years ago

Everyone has reviewed. Feedback that was provided has been addressed. Closing.