Unnecessary uses of providedOptions pattern.

[pixelzoom]

For code review #32 ...

Whether this is something you want to change for this sim is up to you; the work has already been done. But consider this going forward.

If the only purpose of a providedOptions parameter is to pass in 1 value, consider whether you should really be using providedOptions. In many (most?) cases, it's preferrable to simply create a param for that 1 value. This most often occurs with tandem.

For example, in projectile-data-lab-main.js:

  const screens = [
    new VariabilityScreen( { tandem: Tandem.ROOT.createTandem( 'variabilityScreen' ) } ),
    new SourcesScreen( { tandem: Tandem.ROOT.createTandem( 'sourcesScreen' ) } ),
    new MeasuresScreen( { tandem: Tandem.ROOT.createTandem( 'measuresScreen' ) } ),
    new SamplingScreen( { tandem: Tandem.ROOT.createTandem( 'samplingScreen' ) } )
  ];

For the sole purpose of passing tandem via providedOptions, all of these Screen subclasses must:

• Define type SelfOptions
• Define type {Class}Options
• Use optionize.

As an example, this patch shows how much simpler VariabilityScreen.ts becomes if param providedOptions: ProjectileDataLabScreenOptions is replace with tandem: Tandem.

patch

Subject: [PATCH] changed a few color names with MB, https://github.com/phetsims/projectile-data-lab/issues/196
---
Index: js/projectile-data-lab-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/projectile-data-lab-main.ts b/js/projectile-data-lab-main.ts
--- a/js/projectile-data-lab-main.ts	(revision ffc16e88955d57d809691603606394bb2f81379d)
+++ b/js/projectile-data-lab-main.ts	(date 1709233522264)
@@ -33,7 +33,7 @@
   } );
 
   const screens = [
-    new VariabilityScreen( { tandem: Tandem.ROOT.createTandem( 'variabilityScreen' ) } ),
+    new VariabilityScreen( Tandem.ROOT.createTandem( 'variabilityScreen' ) ),
     new SourcesScreen( { tandem: Tandem.ROOT.createTandem( 'sourcesScreen' ) } ),
     new MeasuresScreen( { tandem: Tandem.ROOT.createTandem( 'measuresScreen' ) } ),
     new SamplingScreen( { tandem: Tandem.ROOT.createTandem( 'samplingScreen' ) } )
Index: js/variability/VariabilityScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/variability/VariabilityScreen.ts b/js/variability/VariabilityScreen.ts
--- a/js/variability/VariabilityScreen.ts	(revision ffc16e88955d57d809691603606394bb2f81379d)
+++ b/js/variability/VariabilityScreen.ts	(date 1709233522268)
@@ -6,33 +6,27 @@
  * @author Matthew Blackman (PhET Interactive Simulations)
  */
 
-import Screen, { ScreenOptions } from '../../../joist/js/Screen.js';
-import optionize, { EmptySelfOptions } from '../../../phet-core/js/optionize.js';
+import Screen from '../../../joist/js/Screen.js';
 import projectileDataLab from '../projectileDataLab.js';
 import VariabilityModel from './model/VariabilityModel.js';
 import VariabilityScreenView from './view/VariabilityScreenView.js';
 import ProjectileDataLabStrings from '../ProjectileDataLabStrings.js';
 import VariabilityKeyboardHelpNode from './view/VariabilityKeyboardHelpNode.js';
 import PDLScreenIconFactory from '../common/view/PDLScreenIconFactory.js';
-
-type SelfOptions = EmptySelfOptions;
-
-type ProjectileDataLabScreenOptions = SelfOptions & ScreenOptions;
+import Tandem from '../../../tandem/js/Tandem.js';
 
 export default class VariabilityScreen extends Screen<VariabilityModel, VariabilityScreenView> {
 
-  public constructor( providedOptions: ProjectileDataLabScreenOptions ) {
-
-    const options = optionize<ProjectileDataLabScreenOptions, SelfOptions, ScreenOptions>()( {
-      name: ProjectileDataLabStrings.screen.variabilityStringProperty,
-      homeScreenIcon: PDLScreenIconFactory.createVariabilityScreenIcon(),
-      createKeyboardHelpNode: () => new VariabilityKeyboardHelpNode()
-    }, providedOptions );
-
-    super(
-      () => new VariabilityModel( { tandem: options.tandem.createTandem( 'model' ) } ),
-      model => new VariabilityScreenView( model, { tandem: options.tandem.createTandem( 'view' ) } ),
-      options
+  public constructor( tandem: Tandem ) {
+    super(
+      () => new VariabilityModel( { tandem: tandem.createTandem( 'model' ) } ),
+      model => new VariabilityScreenView( model, { tandem: tandem.createTandem( 'view' ) } ),
+      {
+        name: ProjectileDataLabStrings.screen.variabilityStringProperty,
+        homeScreenIcon: PDLScreenIconFactory.createVariabilityScreenIcon(),
+        createKeyboardHelpNode: () => new VariabilityKeyboardHelpNode(),
+        tandem: tandem
+      }
     );
   }
 }

Again, this is not specific to tandem, but I see it occurring most often with tandem. It is NOT the case that tandem should always be passed via providedOptions, that is an anti-pattern. If you really think there's a chance that you'll need the flexibility of providedOptions, then go for it in those cases. But using providedOptions in every case results in unnecessary code and complexity.

In addition to the Screen subclasses, I see this in ScreenView subclasses, TModel subclasses, and Histogram. There may be other cases.

[samreid]

@matthew-blackman let's discuss this before proceeding.

[samreid]

@matthew-blackman and I discussed this, and we found it preferable to stick with the optionize pattern in these cases, even if only one option (like tandem) is specified, in case another option is added later on--then we don't have to change patterns. Combined with the fact that these have already been written to use optionize, it seems reasonable to keep them for now.