When is it type-safe and appropriate to use the spread operator for combining options?

[samreid]

In https://github.com/phetsims/axon/issues/437#issuecomment-1548810650 it said:

(4) Destructuring should not be used in place of combineOptions, it is not type-safe. For example, this passes a bad option to new Emitter and results in no tsc errors:

const junk = { foobar: true };
const elementAddedEmitter = new Emitter<[ T ]>( {
tandem: options.tandem.createTandem( 'elementAddedEmitter' ),
parameters: [ emitterParameterOptions ],
phetioReadOnly: true,
hasListenerOrderDependencies: options.hasListenerOrderDependencies,
...options.elementAddedEmitterOptions,
...junk
} );

There are a few problems with this.

• First, clarifying terminology for this conversation. "Destructuring" refers to taking elements out of an object like let { a, b, c } = obj; The spread operator is described like so:

The spread (...) syntax allows an iterable, such as an array or string, to be expanded in places where zero or more arguments (for function calls) or elements (for array literals) are expected.

• Second, the example above is type safe. It passes type checking and does not have a type error. However, it does suffer from a related problem which is that it does not perform excess property checking on the elements of junk. I agree we should perform excess property checking in cases like this and I was happy to accept the combineOptions patch in https://github.com/phetsims/axon/issues/437#issuecomment-1548810650. However, this does not mean that when the spread operator is used in an options combination that there will be no excess property checking. For example: foo:'bar' in the object literal is detected as an excess property:

We are using the spread operator in the development of Center and Variability, for instance, see https://github.com/phetsims/center-and-variability/blob/0fc7638eaffe3ef310897fe4668408e1e7a0f3f4/js/mean-and-median/view/MeanAndMedianPlotNode.ts#L29-L33

  public constructor( model: MeanAndMedianModel, sceneModel: CAVSceneModel, providedOptions: MeanAndMedianPlotNodeOptions ) {
    super( model, sceneModel, {
      dataPointFill: 'black',
      ...providedOptions
    } );

I do not feel that code has a type error or any problems with excess property checking. No excess properties can leak in through the object literal, and no excess properties can leak in through the providedOptions, since they are already typed as MeanAndMedianPlotNodeOptions. The main risk would be if someone one day sneaks in another spread object into that expression. But if that happens, it will not fail the type checker unless it has a conflict.

I would also note that TypeScript introduced satisfies in 4.9 which could be used to detect excess properties in spread objects:

But that doesn't seem superior to combineOptions, mainly because you could forget to use it.

My main question is whether we can continue to use the spread operator to combine options in CAV or if it must be converted to combineOptions. @pixelzoom @marlitas @matthew-blackman what do you recommend?

[pixelzoom]

Yes, sorry... spead operator, not destructuring. Point taken.

Imo using the spread operator instead of optionize/combineOptions is lazy programming. I don't like it, and it opens the door for errors. But if that's what you want to do (and promote) and other devs don't have a problem with it, then go for it.

The other point here is that we agreed (as a team) to use specific "options" patterns. We spent a lot of time developing and documenting those patterns. You (@samreid) have decided to do something different with no discussion or documentation. And you're apparently doing that in both common code and sim code. So whether it's typesafe or not, it's confusing (hence this issue and the problems in https://github.com/phetsims/axon/issues/437#issuecomment-1548810650) and does not conform to PhET's current "options" pattern.

[pixelzoom]

  const junk = { foobar: true };
  const elementAddedEmitter = new Emitter<[ T ]>( {
    tandem: options.tandem.createTandem( 'elementAddedEmitter' ),
    parameters: [ emitterParameterOptions ],
    phetioReadOnly: true,
    hasListenerOrderDependencies: options.hasListenerOrderDependencies,
    ...options.elementAddedEmitterOptions,
    ...junk
  } );

Second, the example above is type safe. ...

Please explain how your statement here is true. This example is passing excess properties to new Emitter, with no tsc errors. How is that typesafe?

To clarify... Using the spread operator for options can be typesafe, but does not guarantee type safety like optionize /combineOptions do. One needs to take extra measures to ensure type safety, be aware of the potential problems, and it is possible (as in the above example) to do something that is not typesafe. So why not uniformly use optionize/combineOptions, and avoid the pitfalls of using the spread operator?

[pixelzoom]

We are using the spread operator in the development of Center and Variability

It took me ~30 seconds to introduce a problem that masks a bug:

Subject: [PATCH] convert to TypeScript
---
Index: js/common/model/CAVSceneModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CAVSceneModel.ts b/js/common/model/CAVSceneModel.ts
--- a/js/common/model/CAVSceneModel.ts  (revision ccfef71c8827dc74958b20817614a36cf1e15946)
+++ b/js/common/model/CAVSceneModel.ts  (date 1684419108494)
@@ -103,7 +103,10 @@
     public readonly maxKicksProperty: TReadOnlyProperty<number>,
     maxKicksChoices: number[],
     public kickDistanceStrategy: TKickDistanceStrategy,
-    options: { tandem: Tandem }
+    options: {
+      tandem: Tandem;
+      phetioFeature: true;
+    }
   ) {

     // TODO: should we move styles like this into studio? See https://github.com/phetsims/center-and-variability/issues/117

In the above, I meant to add phetioFeatured: true as a default, but I spelled it incorrectly. Because you're using ...options later in the code, there is no TSC errror, and this problem is undetected. If you were using optionize, this problem would have been detected. And it's disturbing that I see this pattern (anti-pattern?) used throughout CAV.

[samreid]

Imo using the spread operator instead of combineOptions is lazy programming.

Thanks, the reason I raised this issue is to try to understand the reasoning behind this sentiment.

The other point here is that we agreed (as a team) on using specific patterns for "options". We (and I) spent a lot of time developing and documenting those patterns.

At the time we developed combineOptions, we were not aware that the spread operator was a viable alternative. After discovering it and testing it, I feel it is preferable, and if there are no objections we could delete combineOptions altogether.

Please explain how your statement here is true. This example is passing excess properties to new Emitter, with no tsc errors. How its that typesafe?

Thanks for changing my mind! I agree it is not type safe to miss those excess properties.

So if we change the team consensus and documentation, would it be OK to use the spread operator in cases like MeanAndMedianPlotNode.ts described above? Or still lazy?

Two more points about excess property checking:

• First, excess properties are not checked in this code where I added a junk object to the optionize in GuidesNode. Is that a problem:

  export default class GuidesNode extends Node {
  
  public constructor( guides: Guides, armColor: TColor,
                    modelViewTransform: ModelViewTransform2, providedOptions: GuidesNodeOptions ) {
  
  const junk = { foo: 'bar' };
  super( optionize<GuidesNodeOptions, SelfOptions, NodeOptions>()( {
  
    // NodeOptions
    children: [
      new GuideNode( guides.topGuide, armColor, modelViewTransform ),
      new GuideNode( guides.bottomGuide, armColor, modelViewTransform )
    ],
    ...junk
  }, providedOptions ) );
  }
  }

• Second, part I'm confused about and would like to understand better is excess properties get through a lot of places in our code. For instance, if you pause the debugger in Node.ts, the options have a lot more things than just the node options. I'm not seeing where this disconnect is happening.

[pixelzoom]

At the time we developed combineOptions, we were not aware that the spread operator was a viable alternative. After discovering it and testing it, I feel it is preferable, and if there are no objections we could delete combineOptions altogether.

I disagree with this strongly. See https://github.com/phetsims/phet-core/issues/131#issuecomment-1553127735.

Instead, I propose that we ban the spread operator for combining options.

[samreid]

Regarding https://github.com/phetsims/phet-core/issues/131#issuecomment-1553127735, I fixed that in the commit https://github.com/phetsims/center-and-variability/commit/0e9a9cfddfffd5edeaaa0a00a97c21be649d59b1. It was an unrelated problem.

[pixelzoom]

It is most certainly a related problem. Again:

To clarify... Using the spread operator for options can be typesafe, but does not guarantee type safety like optionize /combineOptions do. One needs to take extra measures to ensure type safety, be aware of the potential problems, and it is possible (as in the above example) to do something that is not typesafe. So why not uniformly use optionize/combineOptions, and avoid the pitfalls of using the spread operator?

There are pitfalls to using spread for options. One must understand those pitfalls, and code around them. CAV demonstrates that you do not understand the pitfalls, or haven't taken the time to code around them. And why do this at all when optionize/combineOptions avoid the pitfalls?

[samreid]

Your patch actually changed the parameter type, not an argument. Happy to discuss on zoom.

[pixelzoom]

Your patch actually changed the parameter type, not an argument.

Right. Which is not being detected by TSC.

Here's another example:

Subject: [PATCH] convert to TypeScript
---
Index: js/common/view/CAVScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CAVScreenView.ts b/js/common/view/CAVScreenView.ts
--- a/js/common/view/CAVScreenView.ts   (revision ccfef71c8827dc74958b20817614a36cf1e15946)
+++ b/js/common/view/CAVScreenView.ts   (date 1684420037348)
@@ -13,7 +13,7 @@
 import CAVConstants from '../CAVConstants.js';
 import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
-import { AlignBox, Node } from '../../../../scenery/js/imports.js';
+import { AlignBox, Node, TextOptions } from '../../../../scenery/js/imports.js';
 import EraserButton from '../../../../scenery-phet/js/buttons/EraserButton.js';
 import QuestionBar, { QuestionBarOptions } from '../../../../scenery-phet/js/QuestionBar.js';
 import NumberLineNode from './NumberLineNode.js';
@@ -40,6 +40,7 @@

 type SelfOptions = {
   questionBarOptions: QuestionBarOptions;
+  textOptions?: TextOptions; // we intend to pass these to a Text subcomponent
 };

 export type CAVScreenViewOptions = SelfOptions & ScreenViewOptions;
@@ -72,7 +73,9 @@
   private readonly updateMedianNode: () => void;

   public constructor( model: CAVModel, providedOptions: CAVScreenViewOptions ) {
-    const options = optionize<CAVScreenViewOptions, SelfOptions, ScreenViewOptions>()( {}, providedOptions );
+    const options = optionize<CAVScreenViewOptions, SelfOptions, ScreenViewOptions>()( {
+      textOptions: {}
+    }, providedOptions );

     // View size of a soccer ball
     const objectHeight = 41;
@@ -167,6 +170,7 @@

     this.questionBar = new QuestionBar( this.layoutBounds, this.visibleBoundsProperty, {
       ...options.questionBarOptions,
+      ...options.textOptions,
       tandem: options.tandem.createTandem( 'questionBar' )
     } );
     this.contentLayer.addChild( this.questionBar );

I added textOptions with the intention of passing it to a Text subcomponent. I'm accidentally passing it to QuestionBar. Again, no TSC error. Is this also unrelated?

[samreid]

In discussion with @pixelzoom, we discovered there were masked type errors in Center and Variability. I'll commit one example of that in a moment. We would like to try converting Center and Variability towards mostly optionize and perhaps a few combineOptions and see how it goes.

[samreid]

OK I committed an example of a type error that was being masked by the spread operator in https://github.com/phetsims/center-and-variability/commit/f36330bca5cadf117a36f2611819fb8f80b4e3cb. I'll continue in https://github.com/phetsims/center-and-variability/issues/214 and bring questions back here if there are any. @pixelzoom want to close this issue or eagerly search around other repos or common code for other places that should be converted?

[samreid]

Or we could search for/build a lint rule to detect suspicious cases. Maybe something like "only object literals are allowed when using the spread operator on objects"?

[pixelzoom]

Start by searching for "...options" -- you'll find cases in other repos.

[samreid]

I committed a rule (currently off) that can be used to prohibit the object spread operator in object expressions (arrays ok though).

[samreid]

I enabled the rule for CAV though and it helped me catch all cases.

[samreid]

After discussing this issue, I feel I have a better answer to the question "When is it type-safe and appropriate to use the spread operator for combining options?"

Using the spread operator on an object (not array and not an object literal) disables excess property checking which is undesirable. This applies in any circumstances, including but not limited to options and combining options. We added a lint rule which can prohibit the spread operator for non-array, non-literals used in an object context.

[pixelzoom]

@samreid ready to close?

[samreid]

I wrote up a PSA for next dev meeting. Closing.

[zepumph]

We discussed during dev meeting today. @pixelzoom wasn't part of this conversations, which we feel like was a bit of a bummer because he has great insight on this issue.

Here is the current list of failing cases. Many of them spread directly into an object literal, and seem to be written with a desire to include the type of all keys being spread into the new object literal. The cases where things seem buggy, and where we want this rule, is if it is spreading into a variable with a type, like this:

type X = {
  yMargin: number;
};

const DEFAULT_SEPARATOR_LAYOUT_OPTIONS = {
  stretch: true,
  isSeparator: true
};
const x: X = {
  yMargin: 2,
  ...DEFAULT_SEPARATOR_LAYOUT_OPTIONS // Yay, we 
};

const y: X = {
  yMargin: 2,
  stretch: true, //Yay!!! this should fail
  isSeparator: true
}

Results from chipAway when I turn this on for everything.

• [ ] build-a-nucleus: @Luisav1, @marlitas 3 errors in 2 files.
• [ ] circuit-construction-kit-common: @samreid 1 errors in 1 files.
• [ ] density-buoyancy-common: @jonathanolson 2 errors in 2 files.
• [ ] greenhouse-effect: @jbphet 1 errors in 1 files.
• [ ] joist: @zepumph 1 errors in 1 files.
• [ ] keplers-laws: @AgustinVallejo, @jonathanolson 7 errors in 5 files.
• [ ] my-solar-system: @AgustinVallejo, @jonathanolson 1 errors in 1 files.
• [ ] number-play: @marlitas 3 errors in 1 files.
• [ ] phet-io-wrappers: @zepumph 2 errors in 2 files.
• [ ] quadrilateral: @jessegreenberg 1 errors in 1 files.
• [ ] solar-system-common: @AgustinVallejo, @jonathanolson 1 errors in 1 files.
• [ ] tandem: @zepumph 1 errors in 1 files.

24 errors total.

I would prefer getting this rule to a point where we can turn it on generally, for new code. @samreid @pixelzoom @zepumph will take a look now.

[samreid]

Some quick notes before next discussion:

• The main consideration here is that we want to leverage excess property checking, but that only works on object literals, so we should prefer object literals where possible.

For instance:

type Person = {
  age?: number;
  name: string;
};

const p: Person = {
  name: 'John',
  agee: 42 // Hooray, it caught a typo
};

const otherThing = {
  name: 'John',
  agee: 42
};

const p2: Person = otherThing; // Missed opportunity, did not catch my typo

This is not specific to the spread operator or optionize. For instance, if you make this change:

Subject: [PATCH] Use combineOptions, see https://github.com/phetsims/phet-core/issues/131
---
Index: js/common/model/SoccerBall.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/SoccerBall.ts b/js/common/model/SoccerBall.ts
--- a/js/common/model/SoccerBall.ts (revision c8b518da717305e724a53c706ecb383918b4d3fe)
+++ b/js/common/model/SoccerBall.ts (date 1685108170001)
@@ -72,9 +72,10 @@

   public constructor( public readonly isFirstSoccerBall: boolean, providedOptions: SoccerBallOptions ) {

-    const options = optionize<SoccerBallOptions, SelfOptions, PhetioObjectOptions>()( {
-      phetioState: false
-    }, providedOptions );
+    const myDefaults = {
+      pheti0State: false
+    };
+    const options = optionize<SoccerBallOptions, SelfOptions, PhetioObjectOptions>()( myDefaults, providedOptions );
     super( options );

     this.positionProperty = new Vector2Property( new Vector2( 0, CAVObjectType.SOCCER_BALL.radius ), {

then it will no longer catch that typo.

[samreid]

I'm not too worried about this issue. I feel that the case in circuit construction kit may be appropriate:

    toStateObject: ( resistor: Resistor ): ResistorState => {
      const stateObject = CircuitElement.CircuitElementIO.toStateObject( resistor );
      return {
        ...stateObject,
        resistorType: EnumerationIO( ResistorType ).toStateObject( resistor.resistorType )
      };
    },

I feel it is more about using object literals where possible and knowing that variables and varargs disable excess property checking. I didn't feel confident committing on too many various repos listed above. @pixelzoom or @zepumph how do you want to proceed?

[pixelzoom]

I look at the example in https://github.com/phetsims/phet-core/issues/131#issuecomment-1581795111, and ask "why?" Why did the programmer go to the trouble of defining ResistorState (improperly btw, accordiong to PhET's options pattern) and CircuitElementState? And then use an entirely different pattern in toStateObject to combine the options?

Why not define the types properly, and use them where appropriate? I.e.:

type ResistorStateSelfOptions = {
  resistorType: ResistorType;
};
type ResistorState = ResistorStateSelfOptions & CircuitElementState;

...
    toStateObject: ( resistor: Resistor ): ResistorState => {
      const circuitStateObject = CircuitElement.CircuitElementIO.toStateObject( resistor );
      return optionize<ResistorState , ResistorStateSelfOptions , CircuitElementState>()( {
        resistorType: EnumerationIO( ResistorType ).toStateObject( resistor.resistorType )
      }, circuitStateObject );
    },

[zepumph]

optionize is absolutely not for anything other than the PhET options pattern. There are large assumptions about how you specify each parameter to the function, and how TypeScript should behave. Please do not use optionize anywhere else. If you want another, optionize-like function for cases like this, and combineOptions won't work for some reason, let me know and we can work on something.

Mostly like the best path forward here is a synchronous discussion, because it isn't really even clear to me that we are all agreed in how important future work on this issue is, or if we should even have the lint rule at all.

[pixelzoom]

optionize is absolutely not for anything other than the PhET options pattern. here are large assumptions about how you specify each parameter to the function, and how TypeScript should behave. Please do not use optionize anywhere else. ...

I'm not clear on why this in not an appropriate use of the options pattern. We're assembling an object literal that consists of a set of fields. In general those fields may be optional or required. A state object is a special case where they are all required. If you're saying that the options pattern only applies to the providedOptions parameter for constructors/methods.... What specific "assumptions" are you referring to that prevent its use in other cases?

[zepumph]

optionize makes assumptions about what parameter (based on position) should have include and what behavior it should have. Also, the underlying runtime function, PHET_CORE/merge, is specifically implemented with phet's option pattern in mind (only expanding recursively to items ending in Options. Especially as a maintainer of one of the most complicated files in the code base (optionize), I would really rather not expand its usage beyond the options pattern. It already needs work and doesn't cover all cases well (like nested options).

@samreid and I found a nice way to use _.merge to accomplish the stateObject pattern. We also fixed all other problems, and committed the rule being turned on. I wanted to push on this and do it eagerly, since the majority of these cases were being lazy about options. We can still work on the best pattern for toStateObject composition if you'd like @pixelzoom. See https://github.com/phetsims/circuit-construction-kit-common/blob/ac7789949cfe94d5b254c8961e69d501837ab3b7/js/model/Resistor.ts#L130-L136

@samreid please spot check things.

[samreid]

@zepumph nice work and thanks! I reviewed and these seem like good changes. I'll also add more about excess property checking to the typescript conventions doc.

[samreid]

I added a section on Excess Property Checking to the typescript doc, which is referenced from the code review guidelines. I feel this issue is ready to close. Should we check in with @pixelzoom or any sort of PSA?

[zepumph]

I posted to slack about the new lint rule:

New lint rule committed to the codebase (from dev meeting a while back). no-object-spread-on-non-literals. Spreading objects in typescript does not support excess property checking, so we want to prohibit in general. Please report issues to https://github.com/phetsims/phet-core/issues/131 You may need to restart webstorm after a pull.

Let's hear back from @pixelzoom, especially about https://github.com/phetsims/circuit-construction-kit-common/commit/ac7789949cfe94d5b254c8961e69d501837ab3b7. I think we are close to a close though.

[pixelzoom]

Thanks for elaborating in https://github.com/phetsims/phet-core/issues/131#issuecomment-1590222494. I had forgotten about the constraints on nested options (the requirement for 'Options' suffix). I can't say that I've had any need for nested options in StateObjects, but I get your point. If _.merge is the path forward and provides type-safety, great -- let's document it in https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#io-types.

[zepumph]

Thanks!