Typescript Convention: How to support PhET's option pattern?

[zepumph]

Linked issues:
phetsims/chipper#1049
phetsims/ratio-and-proportion#405
phetsims/ratio-and-proportion#404

@samreid and I spent the last couple hours preparing an extensive example of the ways that options are used throughout the project. We will commit this to a class in Wilder, which currently is passing the ts compiler. From here we can bring this to a group meeting to understand some of the constraints, and potential changes.

In general this felt stable enough that I will take the pattern for a test drive in RAP over in phetsims/ratio-and-proportion#404.

[zepumph]

@samreid how would you like to proceed?

[zepumph]

• Another question. For maintainability, it seems like every common code type should export a custom named options object even if there aren't any options defined by that type. Otherwise subtypes would just the type's supertype options. For example:

Node
Button
RoundButton

If button didn't define any new options, we wouldn't want RoundButton saying that it's options extend NodeOptions, because then if Button adds an option, you would have to change all usage sites. Thus I can imagine this line of code being used in this case;

type ButtonOptions = {} & NodeOptions;

// or maybe this?
type ButtonOptions = NodeOptions;

[zepumph]

I realized that in all the examples we had, everything was a required argument, so I added a subtype that actually supports "options", in the optional sense. @samreid will you please take a look, perhaps we should meet up to discuss these TODOs, but I was surprised that I didn't need to pass in a required argument to the supertype, perhaps I missed something stupid, or didn't use the right Types.

[zepumph]

I'm really happy about this pattern too, similar, but a bit different. It basically is just supporting (5) and (1) from https://github.com/phetsims/wilder/blob/eebf6c69eb068e06f6b5704744b40fc94b2879c0/js/wilder/model/WilderOptionsTypescriptTestModel.ts#L16-L28

Index: js/common/view/RAPTickMarkLabelsNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/RAPTickMarkLabelsNode.ts b/js/common/view/RAPTickMarkLabelsNode.ts
--- a/js/common/view/RAPTickMarkLabelsNode.ts	(revision b321fb1d02d5a131233efb3d94cf5fc2c596fdb1)
+++ b/js/common/view/RAPTickMarkLabelsNode.ts	(date 1635194641823)
@@ -32,11 +32,7 @@
    * @param {Object} [options]
    */
   constructor( tickMarkViewProperty: Property<TickMarkViewType>, tickMarkRangeProperty: Property<number>, height: number,
-               colorProperty: Property<Color | string>, options?: any ) {
-
-    if ( options ) {
-      assert && assert( !options.hasOwnProperty( 'children' ), 'RAPTickMarkLabelsNode sets its own children' );
-    }
+               colorProperty: Property<Color | string>, options?: Omit<NodeOptions, 'children'> ) {
 
     super();
 

[samreid]

Here's a stackoverflow on how to do xor values:

https://stackoverflow.com/questions/44425344/typescript-interface-with-xor-barstring-xor-cannumber

type IFoo = {
  bar: string; can?: never
} | {
    bar?: never; can: number
  };


let val0: IFoo = { bar: "hello" } // OK only bar
let val1: IFoo = { can: 22 } // OK only can
let val2: IFoo = { bar: "hello",  can: 22 } // Error foo and can
let val3: IFoo = {  } // Error neither foo or can

[zepumph]

We discussed this during dev meeting today. @samreid and I got good feedback. I would consider this wilder type our current convention. I made updates and added a TODO in regards to #128. There are now 7 TODOs for this in that file.

[samreid]

As I've tried to apply this pattern in Gravity and Orbits and Bending Light, I'm finding in some cases I need to invert the pattern a bit like so:

type BendingLightScreenViewImplementationOptions = {
  occlusionHandler?: () => void,
  horizontalPlayAreaOffset?: number,
  verticalPlayAreaOffset?: number,
  clampDragAngle?: ( angle: number ) => number,
  ccwArrowNotAtMax?: () => boolean,
  clockwiseArrowNotAtMax?: () => boolean
};

type BendingLightScreenViewOptions = BendingLightScreenViewImplementationOptions & PhetioObjectOptions;

abstract class BendingLightScreenView extends ScreenView {
  //...

  /**
   * @param {BendingLightModel} bendingLightModel - main model of the simulations
   * @param {boolean} laserHasKnob - laser image
   * @param {Object} [providedOptions]
   */
  constructor( bendingLightModel: BendingLightModel, laserHasKnob: boolean, providedOptions?: Partial<BendingLightScreenViewOptions> ) {

    const options = merge( {
      occlusionHandler: () => {}, // {function} moves objects out from behind a control panel if dropped there
      ccwArrowNotAtMax: () => true, // {function} shows whether laser at min angle
      clockwiseArrowNotAtMax: () => true, // {function} shows whether laser at max angle, In prisms tab
      // laser node can rotate 360 degrees.so arrows showing all the times when laser node rotate
      clampDragAngle: ( angle: number ) => angle, // {function} function that limits the angle of laser to its bounds
      horizontalPlayAreaOffset: 0, // {number} in stage coordinates, how far to shift the play area horizontally
      verticalPlayAreaOffset: 0 // {number} in stage coordinates, how far to shift the play area vertically.  In the
                                // prisms screen, it is shifted up a bit to center the play area above the south control panel
}, providedOptions ) as Required<BendingLightScreenViewImplementationOptions & Pick<PhetioObjectOptions, 'tandem'>>;

That is, provide a "type-specific" set of options, which often is the same as "these values can be used in the constructor".

[pixelzoom]

What I'm finding as I'm instrumenting Geometric Optics is that the distincting between options and config is still important. options is what the client provides. config is what we have after merge is called. The TS Utility types Partial and Required are useful for checking options and config.

Here's what I've come up with:

// Optional fields for the client.
// Default values are required to be provided in the constructor via optionDefaults. 
type WidgetOptions = Partial< {
  someOptionalField: number,
  ...
} >;

// config is a collection of all optional and required fields
type WidgetConfig = WidgetOptions & {
  someRequiredField: number,
  ...
};

class Widget {

  constructor( providedConfig: WidgetConfig ) {

    // All optional fields must have a default value.
    const optionDefaults: Required<WidgetOptions> = {
      someOptionalField: 5
    };

    // After merging, we have a complete configuration. 
    // All fields in config are required to have a value.
    const config = merge( optionDefaults, providedConfig } ) as Required<WidgetConfig>;

    …
  }
}

export default Widget;
export { WidgetConfig };

[pixelzoom]

There are other variations of the above, depending on:

• Whether there are any required fields (config) or only optional fields (options)

• Whether the superclass has options/config that needs to be included in the type definition

Here's an example that constrains options to 2 fields, with default values for both of them:

type MyPathOptions = Partial< {
  fill: ColorDef,
  stroke: ColorDef
} >;

class MyPath extends Path {

  constructor( providedOptions: MyPathOptions ) {

    const optionDefaults: Required<MyPathOptions> = {
      fill: ‘white’,
      stroke: ‘black’
    };

    const config = merge( optionDefaults, providedOptions } ) as Required<MyPathOptions>;

    super( …, config );
  }
}

export default MyPath;
export { MyPathOptions };

To allow superclass options in the above example, here's the change needed:

- const config = merge( optionDefaults, providedOptions } ) as Required<MyPathOptions>;
+ const config = merge( optionDefaults, providedOptions } ) as Required<MyPathOptions> & PathOptions;

[pixelzoom]

I'll have to give some more thought to the superclass case. Because I think we'd like to export:

type MyPathOptions = PathOptions & {
  // options that are specific to MyPath
};

But that doesn't work with the rest of the pattern.

[zepumph]

RE: #128, that is basically what we have, we just export that object literal into another type called MyPathDefinedOptions.

[zepumph]

Perhaps we should discuss more at next dev meeting since @pixelzoom has had some time to experiment with the pattern now.

RE: #128

I am not immediately opposed to keeping the naming convention of options/config, but it is completely duplicating what the Type system is already keeping track of, noting, and erroring against. I think it would be pretty straight forward to re-train myself to use typescript to determine if there were required items to pass through an arg object.

[zepumph]

Topic for dev meeting: discuss if we should keep the "config vs. options" naming convention.

[samreid]

Related to #128 it would be good to make sure everyone is aware you can mix required and optional attributes in one type like:

type WidgetConfig = {
  someRequiredField: number,
  someOptionalField?: number
};

I think it would be nice to consider options synonymous with "named arguments", whether they are required or not. One may argue that they are "options" so they must also be "optional". However, we can also think of options as choices. For instance, when you buy a new car, you are given color options. You still have to choose the color (there is no default color when you buy a car) but we can still call it an option.

[pixelzoom]

Yes, I'm aware that optional and required fields can be mixed. Separating them in #128 is required to verify that all optional fields are given default values:

    // All optional fields must have a default value.
    const optionDefaults: Required<WidgetOptions> = {
      someOptionalField: 5
    };

[pixelzoom]

Here are the goals of this pattern, as I understand them:

• Identify fields as optional vs required.
• Verify that the client provides all required fields.
• Verify that default values are provided for all optional fields.
• Verify that all fields have a value after merge.
• Support all of the above for superclass options and nested options.

And maybe:

• Verify that no unsupported fields are present after the merge call.

[zepumph]

There are quire a few more goals, and feel free to note https://github.com/phetsims/wilder/blob/98ba71f7cec505422ba5b6eb2f8fe89a4e0ab7ad/js/wilder/model/WilderOptionsTypescriptTestModel.ts#L15-L27. A couple more though just to have it here:

• Support a Typesafe way to disallow subtypes/usages from providing an option that is set by a constructor.
• Ensure that I can provide a default in my merge call, and potentially use that option in the subtypes constructor that is an option supported/defined by a supertype.

[zepumph]

@samreid and I made good progress on Limitation (I) yesterday. Here is what I said in slack:

PSA to devs: @samreid and I are pushing changes to optionize. Main changes are:

1. optionize no longer accepts a 4th type parameter, it can be inferred from the arguments to the function
2. optionize now has an extra function call as part of it.
3. optionize how only every accepts 2 arguments (at this time), to use the 3-arg version with the first parameter of an empty object, use a function called optionize3.
   Together the changes look like:

optionize<ProvidedOptions, SelfOptions, ParentOptions>()({
  default1: true
}, providedOptions );

optionize3<ProvidedOptions, SelfOptions, ParentOptions>()( {}, {
  default1: true
}, providedOptions );

• We hope to be able to remove optionize3
• We know that Limitation (I) is still not fixed completely. There are two other parts, generally described as:
  • Required options from parent that want to be defined in the defaults of a subtype
  • Nested options up and down the hierarchy.

We will keep cranking away at this as we can.

[jonathanolson]

Noticed this in recent work. Optionize looks like it now creates an additional closure each time (which will probably incentivize avoiding it more in performance sensitive cases). Is there any way to reduce the performance impact and keep the type improvements? I'm a bit concerned about the GC impact, since a closure takes up a lot more than a simple object when created.

[samreid]

Can you please show the additional closure? I'm seeing typical usage cases like:

    const options = optionize<HomeButtonOptions, SelfOptions, JoistButtonOptions>()( {
      highlightExtensionWidth: 4,
      listener: null,

      // pdom,
      containerTagName: 'li',
      descriptionContent: homeScreenDescriptionString,
      appendDescription: true
    }, providedOptions );

which does not create any closures.

UPDATE: Sorry, I was mistaken. The closure is in optionize.ts:

export default function optionize() {
  return (a, b, c) => merge(a, b, c);
}

[samreid]

I removed the closure in the commit. @zepumph and @jonathanolson can you please review?

[zepumph]

Looks great. Thanks.

[jonathanolson]

Looks good to me based on my knowledge of it.

[samreid]

This issue has 136 hidden items, so I would like to split it into side issues as necessary. Aggregating all unchecked boxes and loose threads from above:

• We think that the merge should be renamed, or at least re-understood to be a PhET-specific options-extending pattern, and not a general "merge" implementation. The typing we came up with reflects that. UPDATE: We now use combineOptions for this.
• Experimenting with other ideas to get type inference working on the provided keys?
• Should optionize supply a {} as the first arg to merge by default? Or is that wasteful?
• What to do about typing in Merge, should it stick around?
• Can we apply this patch to prevent accidentally forgetting to pass in providedOptions as the second arg?
• I don't necessarily feel like the var-args point is solved, we only have two cases: optionize( defaults, providedOptions), and optionize( {}, defaults, providedOptions). Do we need to support more?
• I found a case that seems pretty reasonable to keep merge around for, but I want @samreid's opinion on. It has a TODO like "// TODO: how to handle this merge? It seems like PHET_CORE/merge is the best case for this, we are &ing the two arguments into BothHandsPDOMNode, Typescript Convention: How to support PhET's option pattern? Typescript Convention: How to support PhET's option pattern? #128" in Ratio and Proportion
• Let's make sure that in the design pattern it is documented that we prefer the documentation for options to go in the type, not where the default is defined.
• We hope to be able to remove optionize3
• We know that Limitation (I) is still not fixed completely. There are two other parts, generally described as:
  • Required options from parent that want to be defined in the defaults of a subtype
  • Nested options up and down the hierarchy.

@zepumph at your convenience, will you please skim through these and see which can be crossed off or moved to side issues?

[zepumph]

Lots of good work being done here over in #130

[samreid]

@marlitas and I are triaging dev meeting issues, and it seems this issue is getting attention by @zepumph and @chrisklus, especially in #130. So we think it does not need a reminder by being in the "subgroups" column. Please correct me if that's wrong! But moving it to done discussing for now.

UPDATE: On second thought, this seems appropriate for the subgroup column since subgroups will want to work on this during subgroup time.