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

2-param form of optionize allows default values for required properties. #123

Closed
pixelzoom opened this issue Sep 16, 2022 · 4 comments
Closed

Comments

@pixelzoom
Copy link
Contributor

In EqualityExplorerMovable.ts:

type SelfOptions = {
  position: Vector2; // initial position
  dragBounds: Bounds2; // bounds that constrain dragging
  animationSpeed: number; // distance/second when animating
};

export type EqualityExplorerMovableOptions = SelfOptions;
...

export default class EqualityExplorerMovable {
...
  public constructor( providedOptions?: EqualityExplorerMovableOptions ) {

    const options = optionize<EqualityExplorerMovableOptions, SelfOptions>()( {

      // SelfOptions
      position: Vector2.ZERO,
      dragBounds: Bounds2.EVERYTHING,
      animationSpeed: 400
    }, providedOptions );

I've forgotten to indicate that the properties of SelfOptions are all optional, making them required. Then I've provided default values for those required properties, and optionize treats it like it's no problem.

So I suspect there's a problem with the 2-param form of optionize, because this would definitely be a TS error when using the 3-param form.

@samreid
Copy link
Member

samreid commented Sep 28, 2022

If I remove any of the question marks, I see the error triggered. If I remove all question marks, the error goes away. I believe as you suspected, that the default 3rd type parameter is buggy. This can be corrected like so:

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 afcc7b5d10c6953a816320cfe3fa74a24efab061)
+++ b/js/optionize.ts	(date 1664325940694)
@@ -68,7 +68,7 @@
 // KeysUsedInSubclassConstructor = list of keys from ParentOptions that are used in this constructor. Please note that listing required parent option keys that are filled in by subtype defaults is a workaround for Limitation (I).
 export default function optionize<ProvidedOptions,
   SelfOptions = ProvidedOptions,
-  ParentOptions = object>():
+  ParentOptions = EmptySelfOptions>():
   <KeysUsedInSubclassConstructor extends keyof ( ParentOptions )>(
     defaults: HalfOptions<SelfOptions, ParentOptions>,
     providedOptions?: ProvidedOptions
@@ -79,7 +79,7 @@
 // Use this function to gain the typing that optionize provides but in a case where the first argument is an empty object.
 export function optionize3<ProvidedOptions,
   SelfOptions = ProvidedOptions,
-  ParentOptions = object>():
+  ParentOptions = EmptySelfOptions>():
   <KeysUsedInSubclassConstructor extends keyof ( ParentOptions )>(
     emptyObject: ObjectWithNoKeys,
     defaults: HalfOptions<SelfOptions, ParentOptions>,

Making this change reveals 39 type errors. Some of them seem straightforward, such as providing a default for options that are required and hence cannot accommodate a default. I'll convert this issue to a chip-away to address the issues I cannot fix.

UPDATE: Some of the errors do not seem like errors. So there may be more to do in optionize.

@samreid
Copy link
Member

samreid commented Oct 8, 2022

@zepumph can you please comment on this issue and the proposed patch?

@samreid samreid assigned zepumph and unassigned samreid Oct 8, 2022
zepumph added a commit that referenced this issue Mar 9, 2023
@zepumph
Copy link
Member

zepumph commented Mar 9, 2023

This was fixed as part of phetsims/chipper#1360. When I remove the optional part of the parameters:

Subject: [PATCH] add getValidationError for emitter types,
---
Index: js/common/model/EqualityExplorerMovable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/EqualityExplorerMovable.ts b/js/common/model/EqualityExplorerMovable.ts
--- a/js/common/model/EqualityExplorerMovable.ts	(revision b9215b44b57181e04a4a60bb4aaac4301d506b7d)
+++ b/js/common/model/EqualityExplorerMovable.ts	(date 1678390436944)
@@ -23,9 +23,9 @@
 import equalityExplorer from '../../equalityExplorer.js';
 
 type SelfOptions = {
-  position?: Vector2; // initial position
-  dragBounds?: Bounds2; // bounds that constrain dragging
-  animationSpeed?: number; // distance/second when animating
+  position: Vector2; // initial position
+  dragBounds: Bounds2; // bounds that constrain dragging
+  animationSpeed: number; // distance/second when animating
 };
 
 //TODO https://github.com/phetsims/equality-explorer/issues/200 add required tandem to EqualityExplorerMovableOptions

I now get the error. Ready to close?

@pixelzoom
Copy link
Contributor Author

👍🏻 closing.

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

3 participants