Pattern for nestedOptions in optionize

[pixelzoom]

In phetsims/scenery-phet#737 (comment), @samreid asked about this pattern of using optionize where you don't want to require a default value for one or more nested options:

    const options = optionize<TimeControlNodeOptions,
      Omit<SelfOptions, 'playPauseStepButtonOptions' | 'speedRadioButtonGroupOptions'>, NodeOptions>()( {

We discussed this in Slack and decided that Omit<SelfOptions, 'someNestedOptions'> was a good pattern. See discussion below. It sounds like this pattern needs to be communicated to the PhET dev team.

Slack discussion 3/29/2022

Chris Malley 9:22 AM
I’m wondering if providedOptions should generally be providedOptions?: MyClassOptions | null in order to support composition and nested options. I’ve been running into this quite a bit while converting sun/scenery-phet, where it’s impossible to provide a default like (for example) numberDisplayOptions: null , then propagate to new NumberDisplay( .., options.numberDisplayOptions ). (edited)

Michael Kauzmann 9:35 AM
Totally fine with me, but shouldn't that null be internal to the type defining the nested options?

type NumberControlOptions = {
  numberDisplayOptions: NumberDisplayOptions|null
. . .
} (edited) 

[Chris Malley](https://app.slack.com/team/U6EMFARV2)  [9:36 AM](https://phetsims.slack.com/archives/C029UG5BVU3/p1648582579298039)
Yes, but you then can’t pass null to new NumberDisplay.

[Michael Kauzmann](https://app.slack.com/team/U6DFWDG1Z)  [9:37 AM](https://phetsims.slack.com/archives/C029UG5BVU3/p1648582665828029)
Right, and you want to?

[Chris Malley](https://app.slack.com/team/U6EMFARV2)  [9:43 AM](https://phetsims.slack.com/archives/C029UG5BVU3/p1648583022404239)
Here’s the situation:
type SelfOptions = {
  numberDisplayOptions: NumberDisplayOptions|null;
  ...
}

type NumberControlOptions = SelfOptions | NodeOptions;

constructor( ...., providedOptions?: NumberControlOptions ) {

  const options = optionize<NumberControlOptions, SelfOptions, NodeOptions>( {
    numberDisplayOptions: null,
    ...
  }, providedOptions );
  ...

  const numberDisplay = new NumberDisplay( ..., options.numberDisplayOptions );
}
The call to new NumberDisplay is a TS error, because NumberDisplay does not support null for its providedOptions.
The alternative currently is to provide {} as the default, which is wasteful:
type SelfOptions = {
  numberDisplayOptions: NumberDisplayOptions;
  ...
}

const options = optionize<NumberControlOptions, SelfOptions, NodeOptions>( {
    numberDisplayOptions: {},
    ...
  }, providedOptions );
[9:44](https://phetsims.slack.com/archives/C029UG5BVU3/p1648583082918839)
Because numberDisplayOptions is optional, it must have a default value specified in the call to optionize. (edited) 

[Michael Kauzmann](https://app.slack.com/team/U6DFWDG1Z)  [9:45 AM](https://phetsims.slack.com/archives/C029UG5BVU3/p1648583118077479)
Makes sense to me! I thought that we only had cases where that common code that uses nested options would also provide some defaults, meaning there would be an option. I'm totally fine with adding |null to providedOptions, personally. (edited) 

Chris Malley  9:45 AM
null is what we’ve used in the past as that default.
9:47
And add | null as we need them, or add them as a general pattern?  I’m not wild about having to change an API every time I encounter this.  I’d probably change the default to {} and waste an object.

Michael Kauzmann  9:48 AM
I just feel like it isn't that pervasive to warrant it as a general pattern.

Chris Malley  9:49 AM
Btw… If you happen to look at NumberControl.ts, it’s not a good example, because it’s still using merge , was not fully converted to TS.

Michael Kauzmann  9:49 AM
Also, the main reason that I would use the null as a default was because I was about to have a second merge/optionize call below that filled in that null to be an option, but needs other options to do so.
9:50
poking around usages of Options: null, right now to understand better
9:50
My thoughts on what the "classic" example is for why we use null is like so:
https://github.com/phetsims/sun/blob/e9d07497cebc286170b20dfa8c239ad8dc0015c2/js/AccordionBox.ts#L209-L213

Jonathan Olson  9:50 AM
Why would we need nulls in general there, just don't put an option? Is it just to placify optionize?

Chris Malley  9:50 AM
It’s often the case that the class does not fill in the nested options. The nested options are provided so that the client can customize.  For example OopsDialog.js:

      // nested options
      richTextOptions: null,

Michael Kauzmann  9:51 AM
So if there is a case where you don't go back and fill those in, can we just add the |null in the cases we need.
9:51
You are also in the thick of typescript conversion, so I defer to you opinion here. I guess I would most prefer to only add |null when needed.

[Chris Malley](https://app.slack.com/team/U6EMFARV2)  9:52 AM
To answer JO…. Yes, a default value is required by optionize.  If a field is optional, a default value must be specified.

Jonathan Olson  9:52 AM
Then remove it from the things that need defaults from optionize, since it's presumably permitted to be undefined?

Chris Malley  9:53 AM
optionize also will not let you provide a default value for a required field.

Michael Kauzmann  9:53 AM
@samreid just mentioned to me a solution that I prefer best! In Typescript the null default is actually superfluous, because it is redundant to how we declare things in the Options type.
9:53
So can we just delete that spot.

Chris Malley  9:53 AM
Can you clarify? What does “just delete that spot” mean?
9:54
I like JO’s idea of Omit.
9:55
But it sure makes the optionize call look complicated.  Looks like Omit is how JO avoided specifying a default for nested options in NumberControl:
optionize<NumberControlOptions, Omit<SelfOptions, 'numberDisplayOptions' | 'sliderOptions' | 'arrowButtonOptions' | 'titleNodeOptions'>, NodeOptions, 'tandem' >( ... )
(edited)

[Michael Kauzmann](https://app.slack.com/team/U6DFWDG1Z)  9:57 AM
Can we do that for now? Then SR and I could potentially add that to the internals of optionize, where anything that ends in Options does not need to be in the defaults.

[Chris Malley](https://app.slack.com/team/U6EMFARV2)  9:58 AM
I see. That’s what you meant by “just delete that spot”?

[Michael Kauzmann](https://app.slack.com/team/U6DFWDG1Z)  9:59 AM
It is not! But I like JO's solution better.
9:59
Well, I guess it kinda is
9:59
We are really all talking about the same thing in different ways.

Chris Malley  9:59 AM
:+1::skin-tone-2: Thanks for discussing. I’ll try the Omit pattern.

Sam Reid  9:59 AM
To me, the Omit seems preferable to supporting |null everywhere.
:+1::skin-tone-3::+1::skin-tone-2:
2

10:00
One day we may find a way to auto-Omit in optionize based on “*Options” suffix?

[Michael Kauzmann](https://app.slack.com/team/U6DFWDG1Z)  10:00 AM
Especially if it is a convention to factor that type out into a type like
type SelfOptions = . . . .
type SelfOptionsNoNested = . . . . 
:+1::skin-tone-2:
1

</details>

[samreid]

Thanks for bringing this to my attention, I was unclear on how that worked. @zepumph do we need to do anything about this? Is this the desired long-term plan here? Do we need documentation or anything? Please close if there is nothing else to do.

[zepumph]

At this time I do not know if it is possible in Typescript to determine that keys ending in Options should be treated special. Here are some ways I want to try to do it, but that I can't

• In optionize have something like [ x keyof SelfOptions ]: x endsWith( 'Options' )? /* Do something */: never to get a type of just the nested options keys.
• Create an interface like INestedOptions where the interface enforces that the key ends in Options and that the value is an object.

I don't think either are feasible, but if we could figure out something like that, then it would be easy to have the OptionizeDefaults Type exclude requiring a nested options key. In the mean time, what is committed in TimeControlNode sounds really nice.

I created an example with a TODO back to #128 about this in WilderOptionsPatterns. Anything else here for now? I'm going to notify the typescript channel of the (hopefully temporary) convention.

[zepumph]

I pinged slack:

@channel please note a new convention in #127. When using optionize with nested options and there are no defaults to fill in for that nested option key, you need to Omit it from the SelfOptions so that optionize doesn't require you to fill it in with an unhelpful value. This is preferable to having something like myNestedOptions: null as the default (we are in typescript land now). Please note the committed example as well and comment in the issue if you can questions.

[pixelzoom]

Note also that this patterns seems to be most common with nested options, but can apply to any options field.

[samreid]

I don't plan to work on this issue further at the moment. Not sure whether to close or self-unassign. Leaning toward "close", but anyone please reopen for any reason.

[zepumph]

Up until 3 days ago this patter was completely unsolved. We had no way for optionize to know that a nested key was provided. Then I experiemented with @jessegreenberg with a new pattern that I would like feedback on. The main forces that brought me to this code is that Typescript cannot mutate the type of a variable once it is declared, so I wanted the right type to begin with (instead of what we have been doing like const initialOptions = ; .. . . const options = . . .). I also wanted to make sure that each nested option know what was provided, so I could reach into those nested options and use then in the constructor.

@samreid what do you think?

https://github.com/phetsims/joist/blob/e9ecd57dd26ed8195ef00876038773cafd6f2aaa/js/preferences/PreferencesModel.ts#L159-L194

[samreid]

I think that's the way of the future. I want to test drive it in some cases. I don't use nested options too much in my sim-specific code but I should find something.

Maybe convert:

  public constructor( providedOptions?: PreferencesModelOptions ) {
    providedOptions = providedOptions || {};

to

public constructor( providedOptions: PreferencesModelOptions = {} ) {

Also, I discovered the proposed plan is not detecting excess keys. For instance:

Index: js/preferences/PreferencesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/PreferencesModel.ts b/js/preferences/PreferencesModel.ts
--- a/js/preferences/PreferencesModel.ts	(revision a5064217d68bd28986e9b6db82ea9d7df53cc6dc)
+++ b/js/preferences/PreferencesModel.ts	(date 1661480576044)
@@ -169,7 +169,8 @@
         phetioReadOnly: true
       }, providedOptions ) ),
       generalOptions: optionize<GeneralPreferencesOptions>()( {
-        customPreferences: []
+        customPreferences: [],
+        hello: true // We would like this to be a type error, but it isn't
       }, providedOptions.generalOptions ),
       visualOptions: optionize<VisualPreferencesOptions>()( {
         supportsProjectorMode: false,

[samreid]

Also, I tried to apply this pattern in CCK but got this far before I found that it does not flag excess properties:

Index: js/model/ACVoltage.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/ACVoltage.ts b/js/model/ACVoltage.ts
--- a/js/model/ACVoltage.ts	(revision c384d99c91789ee63e20e65c31dea3ae82c97769)
+++ b/js/model/ACVoltage.ts	(date 1661480500635)
@@ -9,6 +9,7 @@
 import BooleanProperty from '../../../axon/js/BooleanProperty.js';
 import NumberProperty from '../../../axon/js/NumberProperty.js';
 import Property from '../../../axon/js/Property.js';
+import { NumberPropertyOptions } from '../../../axon/js/NumberProperty.js';
 import Range from '../../../dot/js/Range.js';
 import optionize, { EmptySelfOptions } from '../../../phet-core/js/optionize.js';
 import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js';
@@ -48,18 +49,21 @@
   public static FREQUENCY_RANGE = new Range( 0.1, 2.0 );
   public static MAX_VOLTAGE_RANGE = new Range( 0, MAX_VOLTAGE );
 
-  public constructor( startVertex: Vertex, endVertex: Vertex, internalResistanceProperty: Property<number>, tandem: Tandem, providedOptions?: ACVoltageOptions ) {
+  public constructor( startVertex: Vertex, endVertex: Vertex, internalResistanceProperty: Property<number>, tandem: Tandem, providedOptions: ACVoltageOptions = {} ) {
     assert && assert( internalResistanceProperty, 'internalResistanceProperty should be defined' );
 
-    const options = optionize<ACVoltageOptions, SelfOptions, VoltageSourceOptions>()( {
-      initialOrientation: 'right',
-      voltage: 9.0,
-      isFlammable: true,
-      numberOfDecimalPlaces: 2,
-      voltagePropertyOptions: {
+    const options = {
+      ...optionize<ACVoltageOptions, SelfOptions, VoltageSourceOptions>()( {
+        initialOrientation: 'right',
+        voltage: 9.0,
+        isFlammable: true,
+        numberOfDecimalPlaces: 2
+      }, providedOptions ),
+
+      voltagePropertyOptions: optionize<NumberPropertyOptions, EmptySelfOptions>()( {
         range: ACVoltage.VOLTAGE_RANGE
-      }
-    }, providedOptions );
+      }, providedOptions.voltagePropertyOptions )
+    };
     super( startVertex, endVertex, internalResistanceProperty, CCKCConstants.BATTERY_LENGTH, tandem, options );
 
     this.maximumVoltageProperty = new NumberProperty( options.voltage, {
Index: js/model/VoltageSource.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/model/VoltageSource.ts b/js/model/VoltageSource.ts
--- a/js/model/VoltageSource.ts	(revision c384d99c91789ee63e20e65c31dea3ae82c97769)
+++ b/js/model/VoltageSource.ts	(date 1661480295564)
@@ -6,8 +6,7 @@
  * @author Sam Reid (PhET Interactive Simulations)
  */
 
-import NumberProperty from '../../../axon/js/NumberProperty.js';
-import Range from '../../../dot/js/Range.js';
+import NumberProperty, { NumberPropertyOptions } from '../../../axon/js/NumberProperty.js';
 import Property from '../../../axon/js/Property.js';
 import Tandem from '../../../tandem/js/Tandem.js';
 import circuitConstructionKitCommon from '../circuitConstructionKitCommon.js';
@@ -23,10 +22,7 @@
 type SelfOptions = {
   initialOrientation?: string; // TODO: enum
   voltage?: number;
-  voltagePropertyOptions?: {
-    range?: Range;
-    tandem?: Tandem;
-  };
+  voltagePropertyOptions?: NumberPropertyOptions;
 };
 export type VoltageSourceOptions = SelfOptions & FixedCircuitElementOptions;
 

[zepumph]

Hmm, strange. I do seem to be getting the excess property checking:

Index: js/preferences/PreferencesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/PreferencesModel.ts b/js/preferences/PreferencesModel.ts
--- a/js/preferences/PreferencesModel.ts	(revision 0f2668684a76091bc771b646ff70faf66643f965)
+++ b/js/preferences/PreferencesModel.ts	(date 1664922787098)
@@ -212,7 +212,8 @@
       }, providedOptions ) ),
       simulationOptions: optionize<SimulationPreferencesOptions, SimulationPreferencesOptions, BaseModelType>()( {
         tandemName: 'simulationModel',
-        customPreferences: []
+        customPreferences: [],
+        hello: true // We would like this to be a type error, but it isn't
       }, providedOptions.simulationOptions ),
       visualOptions: optionize<VisualPreferencesOptions, VisualPreferencesOptions, BaseModelType>()( {
         tandemName: VISUAL_MODEL_TANDEM,

[samreid]

I am seeing the excess property checking in both examples above. Nice! Not sure what was failing before. So should we document and share the new pattern with the team?

    const options = {
      ...optionize<ACVoltageOptions, SelfOptions, VoltageSourceOptions>()( {

[zepumph]

Steps for this issue:

• Create an example in WilderOptionsPatterns (or maybe use one already there) to see that nested optionize calls works well.
• Looks through usages of \wOptions: \{ in sun and scenery-phet to see if we would want use this is some of those spots to gain more type safety. Looking elsewhere seems good, but there are more than 1000 usages on the project, so we may just want to be wary.

[zepumph]

I have been doing a bit more exploring around this feature. Here is what I've found:

1. All usages found with fjdksalfjdskl are not solved by this pattern because they require multiple options calls, since some options are used by defaults to other options (for example Tandem for nested components, or see Slider.ts).
2. Ideally this would get rid of all usages of optionize.*StrictOmit<.*Options', but this isn't really working. The challenge is often that we omit because we don't want to have to provide the defaults of nested options. Perhaps we need to use combineOptions in this case.
3. Maybe this patch is the pattern we are going for defining nested options that are just dependency injected into children. Also maybe we don't even need to mark it as Partial, I need to investigate.

Index: js/wilder/model/WilderOptionsPatterns.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/wilder/model/WilderOptionsPatterns.ts b/js/wilder/model/WilderOptionsPatterns.ts
--- a/js/wilder/model/WilderOptionsPatterns.ts	(revision a9cc0dc442ed8542df4078fb6ca7d8c03909628f)
+++ b/js/wilder/model/WilderOptionsPatterns.ts	(date 1668543602709)
@@ -204,7 +204,7 @@
 ////////
 // Example Three Prime: nested options where you do not provide defaults for them.
 type ItemContainer2Options = {
-  nodeOptions?: ItemOptions;
+  nodeOptions?: Partial<ItemOptions>;
 };
 
 class ItemContainer2 {
@@ -212,8 +212,9 @@
 
   public constructor( providedOptions: ItemContainer2Options ) {
 
-    // TODO: Explicitly omit here until we can work out a way for optionize to detect nested options directly. https://github.com/phetsims/phet-core/issues/128
-    const options = optionize<ItemContainer2Options, StrictOmit<ItemContainer2Options, 'nodeOptions'>>()( {}, providedOptions );
+    const options = optionize<ItemContainer2Options, ItemContainer2Options>()( {
+      nodeOptions: combineOptions<ItemOptions>( providedOptions.nodeOptions || {} )
+    }, providedOptions );
 
     this.node = new Item( options.nodeOptions );
   }

[zepumph]

This is going to need more time to explore. In the meantime if anyone get's bit by nested options + optionize please note in this issue.