Closed pixelzoom closed 5 months ago
@matthew-blackman let's discuss this before proceeding.
@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.
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 usingprovidedOptions
. In many (most?) cases, it's preferrable to simply create a param for that 1 value. This most often occurs withtandem
.For example, in projectile-data-lab-main.js:
For the sole purpose of passing
tandem
viaprovidedOptions
, all of these Screen subclasses must:type SelfOptions
type {Class}Options
optionize
.As an example, this patch shows how much simpler VariabilityScreen.ts becomes if param
providedOptions: ProjectileDataLabScreenOptions
is replace withtandem: Tandem
.patch
```diff 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 ScreenAgain, this is not specific to
tandem
, but I see it occurring most often withtandem
. It is NOT the case that tandem should always be passed viaprovidedOptions
, that is an anti-pattern. If you really think there's a chance that you'll need the flexibility ofprovidedOptions
, then go for it in those cases. But usingprovidedOptions
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.