phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Required options are optional in optionize #130

Closed chrisklus closed 1 year ago

chrisklus commented 1 year ago

@samreid and I were working on omitting ts-expect-error in https://github.com/phetsims/chipper/issues/1366 and found a case where omitting a required super option from the call site option type and then still not including that required option in optionize defaults did not error.

@zepumph was able to take a look with us before having to go and suggested taking a look at example nine in wilder options patterns. We tried using WithOptional like in that case but had no luck. @zepumph could you please take a look again sometime, async or pairing with one of us? Thanks!

Please look for this issue link in code.

chrisklus commented 1 year ago

Patch that @zepumph and I have been experimenting with:

```diff Index: phet-core/js/optionize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/optionize.ts b/phet-core/js/optionize.ts --- a/phet-core/js/optionize.ts (revision 59ec4e8b026d1ddb9e8d3d9a030ca19010243182) +++ b/phet-core/js/optionize.ts (date 1671045305538) @@ -36,16 +36,19 @@ type EmptySelfOptionsKeys = keyof EmptySelfOptions; // This is the type for the `defaults` argument to optionize -export type HalfOptions = +export type HalfOptions = // Everything optional from SelfOptions must have a default specified Omit>, EmptySelfOptionsKeys> & // eslint-disable-line @typescript-eslint/ban-types + // TODO: Anything required in ParentOptions should be required if it wasn't provided through ParentOptions + // TODO: Convey that these keys are not undefined because they were provided in defaults // Any or none of Parent options can be provided - Partial; + Pick; // This is the type for the `defaults` argument to optionize -type OptionizeDefaults = +type OptionizeDefaults = // Everything optional from SelfOptions must have a default specified Omit>, EmptySelfOptionsKeys> & // eslint-disable-line @typescript-eslint/ban-types @@ -53,9 +56,9 @@ // Any or none of Parent options can be provided Partial & - // Any keys provided in KeysUsedInSubclassConstructor are required to have a default provided, with the assumption + // Any keys provided in ParentOptionKeysInDefaults are required to have a default provided, with the assumption // that they will be used from the return type of optionize. - Required>; + Required>; // Factor out the merge arrow closure to avoid heap/cpu at runtime const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => merge( a, b, c, d ); @@ -63,14 +66,14 @@ // ProvidedOptions = The type of this class's public API (type of the providedOptions parameter in the constructor) // SelfOptions = Options that are defined by "this" class. Anything optional in this block must have a default provided in "defaults" // ParentOptions = The public API for parent options, this will be exported by the parent class, like "NodeOptions" -// KeysUsedInSubclassConstructor = list of keys from ParentOptions that are used in this constructor. +// ParentOptionKeysInDefaults = list of keys from ParentOptions that were provided in the defaults. export default function optionize>(): - ( - defaults: HalfOptions, + ( + defaults: HalfOptions, providedOptions?: ProvidedOptions - ) => HalfOptions & ProvidedOptions & Required> { + ) => HalfOptions & ProvidedOptions { return merge4; } @@ -78,11 +81,11 @@ export function optionize3>(): - ( + ( emptyObject: ObjectWithNoKeys, defaults: HalfOptions, providedOptions?: ProvidedOptions - ) => HalfOptions & ProvidedOptions & Required> { + ) => HalfOptions & ProvidedOptions & Required> { return merge4; } @@ -104,12 +107,12 @@ export function optionize4(): - ( + ( emptyObject: ObjectWithNoKeys, defaults1: Partial, defaults2: HalfOptions, providedOptions?: ProvidedOptions - ) => HalfOptions & ProvidedOptions & Required> { + ) => HalfOptions & ProvidedOptions & Required> { return merge4; } @@ -122,21 +125,21 @@ // function optionize(): -// ( +// ( // emptyObject: ObjectWithNoKeys, // defaults: HalfOptions, // providedOptions?: ProvidedOptions -// ) => HalfOptions & ProvidedOptions & Required>; +// ) => HalfOptions & ProvidedOptions & Required>; // // function optionize(): +// ParentOptionKeysInDefaults extends keyof ParentOptions = never>(): // ( // empytObject: ObjectWithNoKeys, -// defaults: OptionizeDefaults, +// defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; +// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; // The implementation gets "any" types because of the above signatures // function optionize() { return ( a: any, b?: any, c?: any ) => merge( a, b, c ); } // eslint-disable-line no-redeclare,bad-text Index: wilder/js/wilder/model/WilderOptionsPatterns.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/wilder/js/wilder/model/WilderOptionsPatterns.ts b/wilder/js/wilder/model/WilderOptionsPatterns.ts --- a/wilder/js/wilder/model/WilderOptionsPatterns.ts (revision abea7107ba9e9d65298e7cd2f593a036f5e58dab) +++ b/wilder/js/wilder/model/WilderOptionsPatterns.ts (date 1671046682693) @@ -271,6 +271,11 @@ options.children.push( new MyItem() ); super( options ); + this.hello( options.children ); + } + + hello( items: Item[] ) { + } } @@ -640,6 +645,40 @@ items.push( new AnotherItem( { x: 5 } ) ); +///////// +// Example Fifteen +// +// + +type GalaxyClassOptions = { + warpSpeed: number; +}; + +class GalaxyClass { + topSpeed: number; + + constructor( options: GalaxyClassOptions ) { + this.topSpeed = options.warpSpeed; + } +} + +type EnterpriseOptions = EmptySelfOptions & StrictOmit; +// type EnterpriseOptions = EmptySelfOptions & GalaxyClassOptions; + +class Enterprise extends GalaxyClass { + + constructor( providedOptions: EnterpriseOptions ) { + + const options = optionize()( { + // warpSpeed: 10 + }, providedOptions ); + + super( options ); + } +} + +const enterpriseD = new Enterprise( {} ); + //////////////////////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////////////////////// @@ -737,6 +776,7 @@ console.log( providedOptions.isRequiredAwesome ); const options = optionize()( { + name: 'hi', // blarg: true, isAwesome: true, // (2) // hasShirt: false, // (3) Index: center-and-variability/js/median/view/MedianScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/js/median/view/MedianScreenView.ts b/center-and-variability/js/median/view/MedianScreenView.ts --- a/center-and-variability/js/median/view/MedianScreenView.ts (revision e554302408018883973ace6db3d867dd584f651e) +++ b/center-and-variability/js/median/view/MedianScreenView.ts (date 1671046682698) @@ -27,7 +27,7 @@ const options = optionize()( { // TODO-TS: Why are isMedianScreen and questionBarOptions optional here? see https://github.com/phetsims/phet-core/issues/130 - isMedianScreen: true, + // isMedianScreen: true, questionBarOptions: { barFill: CAVColors.medianQuestionBarFillColorProperty, questionString: CenterAndVariabilityStrings.medianQuestion @@ -48,6 +48,10 @@ tandem: Tandem.REQUIRED }, providedOptions ); + const hello = options.isMedianScreen; + + type x = MeanOrMedianScreenViewOptions[ 'isMedianScreen' ]; + super( model, options ); } } ```
zepumph commented 1 year ago

@chrisklus and I had a fruitful exploration into trying to solve this particular issue, but it really ended up bleeding into general problems with optionize as a whole. Some takeaways:

  1. It seems like KeysUsedInSubclassConstructor inference is not always correct. It tends to be too general include too many items of ParentOptions that then we think we can use.
  2. Typescript is really annoying and weird in how it treats var?: number as opposed to var: number|undefined. The first one often doesn't get caught when passing var into something that just takes a number.
  3. There is an inherent problem with parts of this issue, where we want to have knowledge about the keys provided in defaults in order to then type check the defaults object. I don't think you can do this with any amount of inference.
  4. We had a really great time using @ts-expect-error in WilderOptionsPatterns in attempt to continue to solidify what the optionize functionality is and should be. Lots more work to do!
  5. AHha! Part of the problem was in how we were tyring to use KeysUsedInSubclassConstructor as an input to OptionizeDefaults, but instead let's just focus on it as part of the return type. How about we try something like this for this github issue:

type DoBetter<SelfOptions, ParentOptions, DefaultOptions, ProvidedOptions> = {
  [X in RequiredKeys<SelfOptions & ParentOptions>]: X extends keyof ( DefaultOptions & ProvidedOptions ) ? ( SelfOptions & ParentOptions )[X] : ( SelfOptions & ParentOptions )[X] | undefined
}

Here is the patch that we will almost certainly never look at again. But could!

```diff Subject: [PATCH] simplify HalfOptions and OptionizeDefaults into the same thing, https://github.com/phetsims/phet-core/issues/130 --- Index: js/types/GracefulPick.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/GracefulPick.ts b/js/types/GracefulPick.ts new file mode 100644 --- /dev/null (date 1671060839850) +++ b/js/types/GracefulPick.ts (date 1671060839850) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * Use PickOptional to pick properties of a type T and make them optional. + * This is useful when picking superclass options that you want to expose in a subclass API. + * (Careful if you pick a required superclass option and make it optional - you'll need to provide a default!) + * It makes life a little easier because you have to fiddle with fewer '<' and '>' characters, + * and PickOptional makes a little more sense than Pick in the context of options. + * + * Example: + * type MyClassOptions = PickOptional; + * Result: + * { stroke?: ColorDef, lineWidth?: number } + * + * @author Chris Malley (PixelZoom, Inc.) + */ +/** + * From T, pick a set of properties whose keys are in the union K + */ +type GracefulPick = { + [P in K]: P extends keyof T ? T[P] : never; +}; + +export default GracefulPick; Index: js/optionize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/optionize.ts b/js/optionize.ts --- a/js/optionize.ts (revision e487ed911b7a778c585e80b682eb586b1abe0fe0) +++ b/js/optionize.ts (date 1671061898769) @@ -18,6 +18,8 @@ import phetCore from './phetCore.js'; import merge from './merge.js'; import IntentionalAny from './types/IntentionalAny.js'; +import GracefulPick from './types/GracefulPick.js'; +import RequiredKeys from './types/RequiredKeys.js'; // https://github.com/piotrwitek/utility-types/blob/master/src/mapped-types.ts type OptionalKeys = { @@ -35,14 +37,22 @@ type EmptySelfOptionsKeys = keyof EmptySelfOptions; +type DefaultParentUsedKeys = keyof ( Exclude, keyof SelfOptions> ); + // This is the type for the `defaults` argument to optionize -type OptionizeDefaults = +type OptionizeDefaults = // Everything optional from SelfOptions must have a default specified Omit>, EmptySelfOptionsKeys> & // eslint-disable-line @typescript-eslint/ban-types - // Any or none of Parent options can be provided - Partial; +// TODO: Anything required in ParentOptions should be required in OptionizeDefault if it wasn't provided through ProvidedOptions +// TODO: Convey that these keys are not undefined because they were provided in defaults +// Any or none of Parent options can be provided + Partial & // initial commit +// Pick // BAD - cares about having required defined in defaults. + Required, RequiredKeys>>; // Whatever we pass in are actually part of the defaults. + // Factor out the merge arrow closure to avoid heap/cpu at runtime const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => merge( a, b, c, d ); @@ -50,14 +60,15 @@ // ProvidedOptions = The type of this class's public API (type of the providedOptions parameter in the constructor) // SelfOptions = Options that are defined by "this" class. Anything optional in this block must have a default provided in "defaults" // ParentOptions = The public API for parent options, this will be exported by the parent class, like "NodeOptions" -// KeysUsedInSubclassConstructor = list of keys from ParentOptions that are used in this constructor. +// ParentOptionKeysInDefaults = list of keys from ParentOptions that are used in this constructor. export default function optionize>(): - ( - defaults: OptionizeDefaults, + >( + defaults: OptionizeDefaults, + RequiredKeys>, keyof ParentOptions>>, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { return merge4; } @@ -65,11 +76,11 @@ export function optionize3>(): - ( + ( emptyObject: ObjectWithNoKeys, defaults: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { return merge4; } @@ -91,12 +102,12 @@ export function optionize4(): - ( + ( emptyObject: ObjectWithNoKeys, defaults1: Partial, defaults2: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { return merge4; } @@ -109,21 +120,21 @@ // function optionize(): -// ( +// ( // emptyObject: ObjectWithNoKeys, // defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => OptionizeDefaults & ProvidedOptions & Required>; +// ) => OptionizeDefaults & ProvidedOptions & Required>; // // function optionize(): +// ParentOptionKeysInDefaults extends keyof ParentOptions = never>(): // ( // empytObject: ObjectWithNoKeys, -// defaults: OptionizeDefaults, +// defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; +// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; // The implementation gets "any" types because of the above signatures // function optionize() { return ( a: any, b?: any, c?: any ) => merge( a, b, c ); } // eslint-disable-line no-redeclare,bad-text
chrisklus commented 1 year ago

Latest patch with TODOs for our next steps!

```diff Index: js/optionize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/optionize.ts b/js/optionize.ts --- a/js/optionize.ts (revision 2f09b71014ea7b926fcfa13ccad40059595adde9) +++ b/js/optionize.ts (date 1671143096500) @@ -18,6 +18,8 @@ import phetCore from './phetCore.js'; import merge from './merge.js'; import IntentionalAny from './types/IntentionalAny.js'; +import GracefulPick from './types/GracefulPick.js'; +import RequiredKeys from './types/RequiredKeys.js'; // https://github.com/piotrwitek/utility-types/blob/master/src/mapped-types.ts type OptionalKeys = { @@ -35,29 +37,44 @@ type EmptySelfOptionsKeys = keyof EmptySelfOptions; +type DefaultParentUsedKeys = keyof ( Exclude, keyof SelfOptions> ); + // This is the type for the `defaults` argument to optionize -type OptionizeDefaults = +type OptionizeDefaults = // Everything optional from SelfOptions must have a default specified Omit>, EmptySelfOptionsKeys> & // eslint-disable-line @typescript-eslint/ban-types - // Any or none of Parent options can be provided - Partial; +// TODO: Anything required in ParentOptions should be required in OptionizeDefault if it wasn't provided through ProvidedOptions +// TODO: Convey that these keys are not undefined because they were provided in defaults +// Any or none of Parent options can be provided + Partial & // initial commit + Pick; + // Factor out the merge arrow closure to avoid heap/cpu at runtime const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => merge( a, b, c, d ); +type DoBetter = { + [X in RequiredKeys]: X extends keyof ( DefaultOptions & ProvidedOptions ) ? ( SelfOptions & ParentOptions )[X] : ( SelfOptions & ParentOptions )[X] | undefined +} + // ProvidedOptions = The type of this class's public API (type of the providedOptions parameter in the constructor) // SelfOptions = Options that are defined by "this" class. Anything optional in this block must have a default provided in "defaults" // ParentOptions = The public API for parent options, this will be exported by the parent class, like "NodeOptions" -// KeysUsedInSubclassConstructor = list of keys from ParentOptions that are used in this constructor. +// ParentOptionKeysInDefaults = list of keys from ParentOptions that are used in this constructor. export default function optionize>(): - ( + + // TODO: try out just returning ParentOptionKeysInDefaults to see what the resulting type is to see if it's inferring correctly + >( defaults: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + // TODO: Make DoBetter figure out required ParentOptions + // TODO: Maybe make this return type more elegant and simpler? + ) => OptionizeDefaults & ProvidedOptions & DoBetter, ProvidedOptions> { return merge4; } @@ -65,11 +82,11 @@ export function optionize3>(): - ( + ( emptyObject: ObjectWithNoKeys, defaults: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { return merge4; } @@ -91,12 +108,12 @@ export function optionize4(): - ( + ( emptyObject: ObjectWithNoKeys, defaults1: Partial, defaults2: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { return merge4; } @@ -109,21 +126,21 @@ // function optionize(): -// ( +// ( // emptyObject: ObjectWithNoKeys, // defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => OptionizeDefaults & ProvidedOptions & Required>; +// ) => OptionizeDefaults & ProvidedOptions & Required>; // // function optionize(): +// ParentOptionKeysInDefaults extends keyof ParentOptions = never>(): // ( // empytObject: ObjectWithNoKeys, -// defaults: OptionizeDefaults, +// defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; +// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; // The implementation gets "any" types because of the above signatures // function optionize() { return ( a: any, b?: any, c?: any ) => merge( a, b, c ); } // eslint-disable-line no-redeclare,bad-text Index: js/types/GracefulPick.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/GracefulPick.ts b/js/types/GracefulPick.ts new file mode 100644 --- /dev/null (date 1671141090504) +++ b/js/types/GracefulPick.ts (date 1671141090504) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * Use PickOptional to pick properties of a type T and make them optional. + * This is useful when picking superclass options that you want to expose in a subclass API. + * (Careful if you pick a required superclass option and make it optional - you'll need to provide a default!) + * It makes life a little easier because you have to fiddle with fewer '<' and '>' characters, + * and PickOptional makes a little more sense than Pick in the context of options. + * + * Example: + * type MyClassOptions = PickOptional; + * Result: + * { stroke?: ColorDef, lineWidth?: number } + * + * @author Chris Malley (PixelZoom, Inc.) + */ +/** + * From T, pick a set of properties whose keys are in the union K + */ +type GracefulPick = { + [P in K]: P extends keyof T ? T[P] : never; +}; + +export default GracefulPick; ```
zepumph commented 1 year ago

I tried returning the type ParentOptionKeysInDefaults, and no matter what I put in for extends, it always gave me the full value, not an inference of just the keys used by the defaults parameter:

ParentOptionKeysInDefaults extends keyof ParentOptions ParentOptionKeysInDefaults extends DefaultParentUsedKeys<SelfOptions, ParentOptions> ParentOptionKeysInDefaults extends OptionizeDefaults<SelfOptions, ParentOptions>

I'm not sure how to get the inference working better.

zepumph commented 1 year ago

I'm sorry to say this has sat for too long. I can't apply the above patch anymore and this will require some real work to figure out. Let's work on it next time we are in this repo. Thanks!

amanda-phet commented 1 year ago

Let's work on it next time we are in this repo.

Should be next month, FYI.

samreid commented 1 year ago

Updated patch:

```diff Subject: [PATCH] Add SimVersion, see https://github.com/phetsims/center-and-variability/issues/104 --- Index: js/types/GracefulPick.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/GracefulPick.ts b/js/types/GracefulPick.ts new file mode 100644 --- /dev/null (date 1682035109066) +++ b/js/types/GracefulPick.ts (date 1682035109066) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * Use PickOptional to pick properties of a type T and make them optional. + * This is useful when picking superclass options that you want to expose in a subclass API. + * (Careful if you pick a required superclass option and make it optional - you'll need to provide a default!) + * It makes life a little easier because you have to fiddle with fewer '<' and '>' characters, + * and PickOptional makes a little more sense than Pick in the context of options. + * + * Example: + * type MyClassOptions = PickOptional; + * Result: + * { stroke?: ColorDef, lineWidth?: number } + * + * @author Chris Malley (PixelZoom, Inc.) + */ +/** + * From T, pick a set of properties whose keys are in the union K + */ +type GracefulPick = { + [P in K]: P extends keyof T ? T[P] : never; +}; + +export default GracefulPick; Index: js/optionize.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/optionize.ts b/js/optionize.ts --- a/js/optionize.ts (revision 1e5d511bc7d6cefee26d342ae7900ce800f9c804) +++ b/js/optionize.ts (date 1682035428413) @@ -20,6 +20,7 @@ import IntentionalAny from './types/IntentionalAny.js'; import RequiredKeys from './types/RequiredKeys.js'; import OptionalKeys from './types/OptionalKeys.js'; +import GracefulPick from './types/GracefulPick.js'; // Gets the parts of an object that are optional type Options = Pick>; @@ -32,8 +33,11 @@ type EmptySelfOptionsKeys = keyof EmptySelfOptions; +type DefaultParentUsedKeys = keyof ( Exclude, keyof SelfOptions> ); + // This is the type for the `defaults` argument to optionize -type OptionizeDefaults = +type OptionizeDefaults = // Everything optional from SelfOptions must have a default specified Omit>, EmptySelfOptionsKeys> & // eslint-disable-line @typescript-eslint/ban-types @@ -41,23 +45,34 @@ // Anything required in the ProvidedOptions should not show up in the "defaults" object { [k in RequiredKeys]?: never; } & +// TODO: Anything required in ParentOptions should be required in OptionizeDefault if it wasn't provided through ProvidedOptions + // TODO: Convey that these keys are not undefined because they were provided in defaults // Any or none of Parent options can be provided - Partial; + Partial & // initial commit + Pick; // Factor out the merge arrow closure to avoid heap/cpu at runtime const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => merge( a, b, c, d ); +type DoBetter = { + [X in RequiredKeys]: X extends keyof ( DefaultOptions & ProvidedOptions ) ? ( SelfOptions & ParentOptions )[X] : ( SelfOptions & ParentOptions )[X] | undefined +}; + // ProvidedOptions = The type of this class's public API (type of the providedOptions parameter in the constructor) // SelfOptions = Options that are defined by "this" class. Anything optional in this block must have a default provided in "defaults" // ParentOptions = The public API for parent options, this will be exported by the parent class, like "NodeOptions" -// KeysUsedInSubclassConstructor = list of keys from ParentOptions that are used in this constructor. +// ParentOptionKeysInDefaults = list of keys from ParentOptions that are used in this constructor. export default function optionize>(): - ( + // TODO: try out just returning ParentOptionKeysInDefaults to see what the resulting type is to see if it's inferring correctly + >( defaults: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + // TODO: Make DoBetter figure out required ParentOptions + // TODO: Maybe make this return type more elegant and simpler? + ) => OptionizeDefaults & ProvidedOptions & DoBetter, ProvidedOptions> { + return merge4; } @@ -65,11 +80,12 @@ export function optionize3>(): - ( + ( emptyObject: ObjectWithNoKeys, defaults: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { + return merge4; } @@ -91,12 +107,13 @@ export function optionize4(): - ( + ( emptyObject: ObjectWithNoKeys, defaults1: Partial, defaults2: OptionizeDefaults, providedOptions?: ProvidedOptions - ) => OptionizeDefaults & ProvidedOptions & Required> { + ) => OptionizeDefaults & ProvidedOptions & Required> { + return merge4; } @@ -109,21 +126,22 @@ // function optionize(): -// ( +// ( // emptyObject: ObjectWithNoKeys, // defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => OptionizeDefaults & ProvidedOptions & Required>; +// ) => OptionizeDefaults & ProvidedOptions & Required>; // // function optionize(): +// ParentOptionKeysInDefaults extends keyof ParentOptions = never>(): // ( // empytObject: ObjectWithNoKeys, -// defaults: OptionizeDefaults, +// defaults: OptionizeDefaults, // providedOptions?: ProvidedOptions -// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; +// ) => ObjectWithNoKeys & OptionizeDefaults & ProvidedOptions; +// // The implementation gets "any" types because of the above signatures // function optionize() { return ( a: any, b?: any, c?: any ) => merge( a, b, c ); } // eslint-disable-line no-redeclare,bad-text ```
samreid commented 1 year ago

Fixed in the commits. I'll add ts-expect-error for the type errors that were masked by this bug so we can chip away.

samreid commented 1 year ago

Results from chipAway:

We have corrected a deficiency in optionize where it didn't correctly error when you forgot to provide an option that was required by the parent options and not provided in the provided options. Please see https://github.com/phetsims/phet-core/commit/449042e65311361766b8dbc3a06e0f72496c1ff4 for the correction to optionize and see https://github.com/phetsims/circuit-construction-kit-common/commit/8572344194cf584efd36bfe153c0023bc3b7d22b for an example fix. The above 20 errors are ts-expect-error at the moment. You can find the erroneous lines by searching for this issue url https://github.com/phetsims/phet-core/issues/130. UPDATE: Even better fix below: https://github.com/phetsims/sound/commit/3dc8fb0aa21213dae2a227c3f24e0fccfba12f09

pixelzoom commented 1 year ago

@samreid Any reason why this issue has not been migrated to phet-core, where optionize lives? It is not specific to center-and-variability.

pixelzoom commented 1 year ago

@samreid I'm confused about how you were able to add @ts-expect-error comments to geometric-optics, natural-selection, ph-scale, and scenery-phet. All of those repos add ESlint rule "@typescript-eslint/ban-ts-comment" in package.json, which should prohibit @ts-expect-error comments. Thoughts on why this did not fail?

samreid commented 1 year ago

For number play, I think a good solution might be something like this:

```diff Subject: [PATCH] Fix optionize type parameters, see https://github.com/phetsims/phet-core/issues/130 --- Index: js/common/view/NumberPlayScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/NumberPlayScreenView.ts b/js/common/view/NumberPlayScreenView.ts --- a/js/common/view/NumberPlayScreenView.ts (revision b8b04f93397cbacede123db8910560a7de9a75f7) +++ b/js/common/view/NumberPlayScreenView.ts (date 1682076413239) @@ -82,12 +82,10 @@ // create and add the WordAccordionBox const wordAccordionBox = new WordAccordionBox( model.currentNumberProperty, - wordAccordionBoxHeightProperty, - - // @ts-expect-error - chip away for https://github.com/phetsims/phet-core/issues/130 - optionize()( { - expandedProperty: this.wordAccordionBoxExpandedProperty - }, options.wordAccordionBoxOptions ) ); + wordAccordionBoxHeightProperty, { + expandedProperty: this.wordAccordionBoxExpandedProperty, + ...options.wordAccordionBoxOptions + } ); wordAccordionBox.left = this.layoutBounds.minX + NumberSuiteCommonConstants.ACCORDION_BOX_MARGIN_X; wordAccordionBox.top = this.layoutBounds.minY + NumberSuiteCommonConstants.SCREEN_VIEW_PADDING_Y; this.addChild( wordAccordionBox ); @@ -95,12 +93,10 @@ // create and add the TotalAccordionBox const totalAccordionBox = new TotalAccordionBox( model.onesCountingArea, - options.upperAccordionBoxHeight, - - // @ts-expect-error - chip away for https://github.com/phetsims/phet-core/issues/130 - optionize()( { - expandedProperty: this.totalAccordionBoxExpandedProperty - }, options.totalAccordionBoxOptions ) ); + options.upperAccordionBoxHeight, { + expandedProperty: this.totalAccordionBoxExpandedProperty, + ...options.totalAccordionBoxOptions + } ); totalAccordionBox.centerX = this.layoutBounds.centerX; totalAccordionBox.top = wordAccordionBox.top; this.addChild( totalAccordionBox ); @@ -109,12 +105,10 @@ const tenFrameAccordionBox = new TenFrameAccordionBox( model.currentNumberProperty, model.sumRange, - options.upperAccordionBoxHeight, - - // @ts-expect-error - chip away for https://github.com/phetsims/phet-core/issues/130 - optionize()( { - expandedProperty: this.tenFrameAccordionBoxExpandedProperty - }, options.tenFrameAccordionBoxOptions ) ); + options.upperAccordionBoxHeight, { + expandedProperty: this.tenFrameAccordionBoxExpandedProperty, + ...options.tenFrameAccordionBoxOptions + } ); tenFrameAccordionBox.right = this.layoutBounds.maxX - NumberSuiteCommonConstants.ACCORDION_BOX_MARGIN_X; tenFrameAccordionBox.top = wordAccordionBox.top; this.addChild( tenFrameAccordionBox ); ```

For SpectrumSlider, a solution like this seems reasonable to me:

```diff Subject: [PATCH] Fix optionize type parameters, see https://github.com/phetsims/phet-core/issues/130 --- Index: js/SpectrumSlider.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/SpectrumSlider.ts b/js/SpectrumSlider.ts --- a/js/SpectrumSlider.ts (revision fd8c6c7ce3c657de806f1298286ba76f9ebecb43) +++ b/js/SpectrumSlider.ts (date 1682076850560) @@ -78,6 +78,9 @@ type ParentOptions = AccessibleSliderOptions & NodeOptions; export type SpectrumSliderOptions = SelfOptions & StrictOmit; +const DEFAULT_MIN_VALUE = 0; +const DEFAULT_MAX_VALUE = 1; + /** * @deprecated use WavelengthNumberControl, or Slider.js with SpectrumSliderTrack and SpectrumSliderTrack, * see https://github.com/phetsims/scenery-phet/issues/729 @@ -94,12 +97,15 @@ assert && deprecationWarning( 'SpectrumSlider is deprecated, please use Slider with SpectrumSlideTrack/Thumb instead' ); // options that are specific to this type - // @ts-expect-error - chip away for https://github.com/phetsims/phet-core/issues/130 const options = optionize()( { + // ParentOptions + valueProperty: valueProperty, + enabledRangeProperty: new Property( new Range( providedOptions?.minValue ?? DEFAULT_MIN_VALUE, providedOptions?.maxValue ?? DEFAULT_MAX_VALUE ) ), + // SelfOptions - minValue: 0, - maxValue: 1, + minValue: DEFAULT_MIN_VALUE, + maxValue: DEFAULT_MAX_VALUE, valueToString: ( value: number ) => `${value}`, valueToColor: ( value: number ) => new Color( 0, 0, 255 * value ), @@ -146,13 +152,6 @@ // validate values assert && assert( options.minValue < options.maxValue ); - // mix accessible slider functionality into HSlider - assert && assert( !options.valueProperty, 'SpectrumSlider sets its own valueProperty' ); - options.valueProperty = valueProperty; - - assert && assert( !options.enabledRangeProperty, 'SpectrumSlider sets its own enabledRangeProperty' ); - options.enabledRangeProperty = new Property( new Range( options.minValue, options.maxValue ) ); - // These options require valid Bounds, and will be applied later via mutate. const boundsRequiredOptionKeys = _.pick( options, Node.REQUIRES_BOUNDS_OPTION_KEYS ); ```
pixelzoom commented 1 year ago

Slider and FaucetNode have similar problems, related to trait AccessibleSlider and options valueProperty and enabledRangeProperty. I've created separate issues for those, assigned to @zepumph and @jessegreenberg.

My repos are done, so self unassigning.

chrisklus commented 1 year ago

@samreid and I took care of Number Play's together, thanks! We think this is all good to close.