phetsims / phet-core

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

Optionize doesn't always catch illegal extra options #116

Closed samreid closed 1 year ago

samreid commented 2 years ago

Discovered in https://github.com/phetsims/scenery-phet/issues/730#issuecomment-1091951996 and also mentioned in https://github.com/phetsims/center-and-variability/blob/ddfb7ee92244194cfed0e8ad58a63ab212f8bca9/js/median/view/MedianScreenView.ts#L25-L32

Adding new options in those sites, like:

Index: js/diffraction/view/DiffractionScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/diffraction/view/DiffractionScreenView.js b/js/diffraction/view/DiffractionScreenView.js
--- a/js/diffraction/view/DiffractionScreenView.js  (revision c5a7634fc2a06be2e4813eaf61b99e2b30ccfa56)
+++ b/js/diffraction/view/DiffractionScreenView.js  (date 1649436797187)
@@ -259,7 +259,8 @@
         arrowButtonOptions: {
           touchAreaXDilation: WaveInterferenceConstants.NUMBER_CONTROL_HORIZONTAL_TOUCH_AREA_DILATION,
           touchAreaYDilation: 7
-        }
+        },
+        foo: true
       } ), merge( {}, PANEL_OPTIONS, {
         left: laserPointerNode.left,
         top: apertureScaleIndicatorNode.top,

Does not yield an "unsupported option key" type error like it should.

image
samreid commented 2 years ago

Tagging for https://github.com/phetsims/phet-core/issues/128

zepumph commented 1 year ago

I am so incredibly perplexed by this. I thought it may have to do with the nested strict omit call, but I cannot get things to error in any way.

Subject: [PATCH] add getValidationError for emitter types,
---
Index: scenery-phet/js/WavelengthNumberControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/WavelengthNumberControl.ts b/scenery-phet/js/WavelengthNumberControl.ts
--- a/scenery-phet/js/WavelengthNumberControl.ts    (revision 050d2abb757d9fb08f9a2f251f0cf2c2f6c0a1a0)
+++ b/scenery-phet/js/WavelengthNumberControl.ts    (date 1678391802147)
@@ -36,8 +36,8 @@
   spectrumSliderThumbOptions?: SpectrumSliderThumbOptions;
 };

-export type WavelengthNumberControlOptions = SelfOptions &
-  NestedStrictOmit<NumberControlOptions, 'sliderOptions', 'trackNode' | 'thumbNode'>;
+export type WavelengthNumberControlOptions = SelfOptions & NumberControlOptions;
+  // NestedStrictOmit<NumberControlOptions, 'sliderOptions', 'trackNode' | 'thumbNode'>;

 /**
  * @param wavelengthProperty - wavelength, in nm
Index: wave-interference/js/diffraction/view/DiffractionScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/wave-interference/js/diffraction/view/DiffractionScreenView.ts b/wave-interference/js/diffraction/view/DiffractionScreenView.ts
--- a/wave-interference/js/diffraction/view/DiffractionScreenView.ts    (revision e2f341985c820735381fb660e60253c0617c33f5)
+++ b/wave-interference/js/diffraction/view/DiffractionScreenView.ts    (date 1678391848582)
@@ -15,7 +15,7 @@
 import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js';
 import LaserPointerNode from '../../../../scenery-phet/js/LaserPointerNode.js';
 import VisibleColor from '../../../../scenery-phet/js/VisibleColor.js';
-import WavelengthNumberControl from '../../../../scenery-phet/js/WavelengthNumberControl.js';
+import WavelengthNumberControl, { WavelengthNumberControlOptions } from '../../../../scenery-phet/js/WavelengthNumberControl.js';
 import { Circle, HBox, Image, LinearGradient, Node, Path, Rectangle, VBox } from '../../../../scenery/js/imports.js';
 import RectangularRadioButtonGroup from '../../../../sun/js/buttons/RectangularRadioButtonGroup.js';
 import ToggleNode from '../../../../sun/js/ToggleNode.js';
@@ -248,12 +248,25 @@
     model.onProperty.linkAttribute( incidentBeam, 'visible' );
     model.onProperty.linkAttribute( transmittedBeam, 'visible' );

+    const x: WavelengthNumberControlOptions = {
+      arrowButtonOptions: {
+        touchAreaXDilation: WaveInterferenceConstants.NUMBER_CONTROL_HORIZONTAL_TOUCH_AREA_DILATION,
+        touchAreaYDilation: 7
+      },
+      foo: 'hi', // wrong key
+      range: 'hii', // should be range
+      title: 5 // should be string
+    };
+
     const wavelengthPanel = new WaveInterferencePanel(
       new WavelengthNumberControl( model.wavelengthProperty, {
         arrowButtonOptions: {
           touchAreaXDilation: WaveInterferenceConstants.NUMBER_CONTROL_HORIZONTAL_TOUCH_AREA_DILATION,
           touchAreaYDilation: 7
-        }
+        },
+        foo: 'hi', // wrong key
+        range: 'hii', // should be range
+        title: 5 // should be string
       } ), merge( {}, PANEL_OPTIONS, {
         left: laserPointerNode.left,
         top: apertureScaleIndicatorNode.top,
samreid commented 1 year ago

This is very embarrassing. Many files in Wave Interference have ts-nocheck and when that is removed, the errors are caught. This is not a problem with optionize or anything. It is a problem that Wave Interference is half-baked TS.

image

Closing.