Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionize3 should allow some defaults from the first arg #118

Open
Tracked by #129
zepumph opened this issue May 13, 2022 · 4 comments
Open
Tracked by #129

Optionize3 should allow some defaults from the first arg #118

zepumph opened this issue May 13, 2022 · 4 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 13, 2022

@pixelzoom and I were talking over in phetsims/utterance-queue#81 (comment) (in a zoom call), and it would be so very nice if we could have this code working in FocalLengthControl:

Index: js/common/view/FocalLengthControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/FocalLengthControl.ts b/js/common/view/FocalLengthControl.ts
--- a/js/common/view/FocalLengthControl.ts	(revision 64361330d8e6eebc21fc027b54db129af1b5ac95)
+++ b/js/common/view/FocalLengthControl.ts	(date 1652466527269)
@@ -17,9 +17,11 @@
 import IReadOnlyProperty from '../../../../axon/js/IReadOnlyProperty.js';
 import { NodeOptions } from '../../../../scenery/js/imports.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
-import optionize, { combineOptions3 } from '../../../../phet-core/js/optionize.js';
+import { combineOptions3, optionize3 } from '../../../../phet-core/js/optionize.js';
 
-type SelfOptions = {};
+type SelfOptions = {
+  bar?: number;
+};
 
 type FocalLengthControlOptions = SelfOptions & PickRequired<NodeOptions, 'tandem'>;
 
@@ -66,8 +68,9 @@
     } );
 
     // Now add providedOptions to the defaults.
-    const options = optionize<FocalLengthControlOptions, SelfOptions, NumberControlOptions>()(
-      numberControlDefaults, providedOptions );
+    const options = optionize3<FocalLengthControlOptions, SelfOptions, NumberControlOptions>()( {
+      bar: 6
+    }, numberControlDefaults, providedOptions );
 
     super( textProperty.value, focalLengthMagnitudeProperty, range, options );
 

@samreid, it actually doesn't seem like too much of an issue to & the first two args to gether to make the defaults happen. If that occurs inside of Optionize, then we are in luck and won't need too many duplicate lines calling optionize.

@zepumph
Copy link
Member Author

zepumph commented May 13, 2022

Perhaps https://stackoverflow.com/questions/66426230/how-to-enforce-a-type-for-multiple-arguments-in-typescript will be helpful. We can look at the type of both arguments to determine that together they include all defaults.

@samreid, do you think this is possible?

@zepumph
Copy link
Member Author

zepumph commented May 13, 2022

If this was possible, it would be a game changer, and basically fix the issues with how merge differs from optionize. We could event make optionize5:

type SelfOptions = {
  foo?: number;
  bar?: boolean;
  other?: string;
}
const options = optionize5<ProvidedOptions, SelfOptions>()( { foo: 5 }, { bar: true }, { other: 'hi' }, {}, providedOptions );

@samreid
Copy link
Member

samreid commented Aug 27, 2022

It seems we need a type that says T1 & T2 & T3 = U and T1, T2 and T3 can split it up however they want. I don't know a way to do that and I don't understand the stackoverflow idea. But maybe this would be possible using object spread?

But that doesn't flag excess properties:

    const options = optionize<FocalLengthControlOptions, SelfOptions, NumberControlOptions>()( {
      bar: 6,
      ...numberControlDefaults,

      test: 'fake'
    }, providedOptions );

Want to add this to one of our Tuesday agendas?

@samreid samreid removed their assignment Aug 27, 2022
@zepumph
Copy link
Member Author

zepumph commented Mar 9, 2023

This will be quite a bit of work to investigate. It should be handled as part of our next optionize sprint.

@zepumph zepumph removed their assignment Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants