Should optionize 2nd argument really default to {} ?

[pixelzoom]

In WilderOptionsPattern.ts:

    // In the simplest case, optionize just takes the options that this class defines.
    const options = optionize<ItemOptions, ItemOptions>( {

This case keeps biting me, due to what feels like unnecessary duplication. Why does the second arg (SelfOptions) need to default to {}? Is there a reason why it can't default to the value of the first arg? So that the above is simply:

    const options = optionize<ItemOptions>( {

[zepumph]

It is buggy for the second arg to be the same as the first if there are any parent optional options. If that is the case, then by defaulting the self options to the providedOptions, it will require you to fill in defaults for all optional options from your parents as well.

Since the majority of cases in the project will occur in subtypes with parent options, I'm hesitant to make this change. Does that make sense? Did I understand the question correctly?

---

I think there is a good chance that this issue is going to relate heavily to what @samreid said in https://github.com/phetsims/phet-io/issues/1843#issuecomment-1049479690,
because if you are consistently hiding the majority of parent options from the public api (i.e. providedOptions), then this is much less of a worry, and most likely you have a default for every parent option that you are allowing in the public api anyways. The worry is that the second arg defaulting to the providedOptions type would be too much of a "gotcha" when it errors out for not providing defaults for parent options. The "gotcha" in your case though is that you don't get validation on providing defaults for any ItemOptions. I'm a bit torn, and would like to hear @pixelzoom respond.

Happy to talk it out too.

[pixelzoom]

It is buggy for the second arg to be the same as the first if there are any parent optional options. I

I don't follow how that it relevant to this use case. ParentOptions is the 3rd type argument to optionize, and is {} in this use case. So there cannot possibly be any "parent optional options".

[pixelzoom]

Here's an example in GO LensShapes.ts:

type LensShapesOptions = {
  isHollywooded?: boolean,
  offsetRadius?: number
};

class LensShapes implements OpticShapes {
  ...
  constructor( ..., providedOptions?: LensShapesOptions ) {
     ...
    const options = optionize<LensShapesOptions, LensShapesOptions>( {
      isHollywooded: true
      offsetRadius: 100
    }, providedOptions );

There is no superclass involved. All that I'm doing is filling in default values for a couple of optional options that are specific to this class. Why is it necessary to specify LensShapeOptions twice in optionize<LensShapesOptions, LensShapesOptions>?

[zepumph]

I don't follow how that it relevant to this use case. ParentOptions is the 3rd type argument to optionize, and is {} in this use case. So there cannot possibly be any "parent optional options".

I don't mean the 3rd arg, which we don't need to discuss in this part, I mean when any options from the parent are part of the providedOptions type.

RE your code in #105 (comment):

Yes, exactly. You gave the perfect example of the side where self options could default to providedOptions. Here is the spot where it could be confusing:

import optionize from '../phet-core/js/optionize.js';

class OpticShapes {}

type OpticShapesOptions = {
  superOption1?: number,
  superOption2?: number,
  superOption3?: number,
}

type LensShapesSelfOptions = {
  isHollywooded?: boolean,
  offsetRadius?: number
};

type LensShapesOptions = LensShapesSelfOptions & OpticShapesOptions;

class LensShapes implements OpticShapes {

  constructor( providedOptions?: LensShapesOptions ) {
    
    // No error in this implementation
    // const options = optionize<LensShapesOptions, LensShapesSelfOptions, OpticShapesOptions>( {
    //   isHollywooded: true,
    //   offsetRadius: 100
    // }, providedOptions );

    // Here notice that it will get mad at you because it expects all the optional parent options to be filled in here.
    const options = optionize<LensShapesOptions, LensShapesOptions>( {
      isHollywooded: true,
      offsetRadius: 100
    }, providedOptions );
  }
}

Now that there are parent options, using them in providedOptions (which by default passes them into the self options spot) will error out saying you need to set defaults for those parent options in this subtype.

Again, still torn here, just trying to show the tradeoff, and wonder what the better default is. If we implement the way that @pixelzoom recommends, I think you will hit a red error more often, but perhaps that louder error may be helpful in reminding you to fill in self options to be narrower than providedOptions.

@samreid do you have thoughts?

[pixelzoom]

Boy am I confused. I'm trying to get at a case where there are no ParentOptions, and you're converting it to a case that has ParentOptions. I still don't understand why that's relevant.

And in your example:

const options = optionize<LensShapesOptions, LensShapesOptions>( {

Why would you use that form in this case? That seems just plain wrong wrong. There are no SelfOptions, and there are ParentOptions, so isn't this the correct form?:

const options = optionize<LensShapesOptions, {}, OpticShapesOptions>( {

[zepumph]

I've been convinced enough to try it out. I'll let you know if things get complicated in any cases as I convert.

[zepumph]

So I think this is probably correct, but it is a behavior change that I think is worth a PSA about. @pixelzoom please review, and note the changes to Ratio and Proportion and Center and Spread in simple usages of optionize. Do you still agree with this change?

[pixelzoom]

Slack:

Chris Malley [11:25 AM]
I don’t understand why an explict {} had to be added for SelfOptions in some cases, like in RatioHandNode.ts:
const options = optionize<PathOptions,{}>

Michael Kauzmann [11:25 AM]
Since that used to be the default, if you don't add that, then optionize thinks that call to optionize MUST provide defaults for every optional item in PathOption.

The second type arg determines what options require defaults in the first value argument object. (edited)

I won't add it to dev meeting, we can keep discussing.

[pixelzoom]

@zepumph and I discussed my remaining questions via Zoom - thanks! I'm happy with where this is at, and understand what's going on. So closing.

[zepumph]

Using this regex, I realized how many usages there still are of specifying the second parameter type (the SelfOptions) by duplicating the first param:

optionize<(\w+), \1

For example:
https://github.com/phetsims/counting-common/blob/9e4b4d8840f508618e07e50428eeeb1282350166/js/common/view/PaperNumberNode.ts#L70
https://github.com/phetsims/axon/blob/876368b6a281721e91ae276cdac15dfa37e11137/js/EnabledComponent.ts#L53

Reopening

(note I'm not good at regex, I just found the "Repetition and Backreferences" section in https://www.regular-expressions.info/backref.html)

[samreid]

Can you help me understand or add a code comment why SelfOptions = ProvidedOptions is a reasonable default, or what the ramifications of that are?

[zepumph]

Yes. Added doc above. Basically @pixelzoom and I have been spending the last year trying to figure out what the best "default" optionize case is. For a while, we said that by default none of the keys needed defaults, thus SelfOptions was an empty object. Then we talked about/experimented with similar things in Parent options. By landing on SelfOptions by default taking the value of ProvidedOptions, the simple case is clearer, and doesn't require you to repeat the parameter twice (i.e. https://github.com/phetsims/wilder/blob/75388abc6e07721e91eca9ac7f7e3111cbb2d606/js/wilder/model/WilderOptionsPatterns.ts#L86-L103).

Ready to close?

[samreid]

Sounds great, thanks! Closing.