Consider using _.merge (or similar pattern) for nested options instead of _.extend

[mbarlow12]

After an unexpectedly lengthy thread in Slack, it seemed like this was worth discussing at the dev meeting.

While working on phetsims/scenery-phet#451 and implementing the nested options pattern outlined in https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#nesting, I realized that Lodash has a merge function that has similar functionality to _.extend though can also extend nested objects.

function MyNodeTypeWithHSliderInIt( options ) {

    options = _.merge( {
      visible: false,
      pickable: false,
      
      hsliderOptions: {
        endDrag: function() { console.log( 'Drag Ended') }, 
        startDrag: function() { console.log( 'Drag Started') }
      }
    }, options );
  
    var slider = new HSlider( new Property(), new Range(), options.hsliderOptions );
  }

If the client overwrites hsliderOptions.startDrag and adds hsliderOptions.ariaValueTextPattern = somePatternString, those changes will all be correctly passed to the HSlider constructor without an additional call to _.extend.

However, 2 concerns arise from this function:

1. it mutates the first argument (same behavior as extend, so if a reference to a default object were passed in (_.merge( defaultOptions, otherOptions )), defaultOptions would have the new/updated keys added. From Slack, @samreid noted that this can be solved with

A note, we may typically need to do _.merge({}, DEFAULTS, {other things}) so we don’t mess up the DEFAULTS

2. Unlike _.extend, _.merge performs a deep copy of the objects. Developers noted that this is a problem for situations like Enumerations where the actual reference is important.

@jonathanolson

Merge sounds like it would prevent us from being able to pass plain JS objects through. For instance... enumeration values
sounds like it would mutate the name of enumeration values as currently written.

_.merge( {}, { repr: Repr.PIE } ).repr === Repr.PIE // false

or

const obj = _.merge( {}, { nodeOption: nodeOpt, A: enumOpt.A } );
Object { nodeOption: {…}, A: {…} }
obj.A === enumOpt.A;
false
obj.nodeOption === nodeOpt
true

So there are

@samreid mentioned

If we like the idea of merge, but lodash isn’t a perfect fit, we could create PHET_CORE/merge however we like.

@jonathanolson

merge( /*start template*/ { subopts: {} } /*end template*/, { subopts: { a: 5 } }, { subopts: { b: 2 } } )

[samreid]

@jonathanolson can you elaborate on your point? I don't understand it.

[samreid]

So the main issue moving forward is: we like the recursion, but we need control about how deep the recursion should go? And for Enumerations we can maybe control the depth of recursion with freeze but we have no way to control the depth of recursion with, say, Node?

Maybe if we invent our own PHET_CORE/Merge or PHET_CORE/Options we would determine recursion by a key name convention. For instance, it only recurses into sub-objects that end in the suffix options, like sliderOptions.

[mbarlow12]

For instance, it only recurses into sub-objects that end in the suffix options, like sliderOptions.

This seems like a nice pattern. I'm starting to lean towards a PHET_CORE/Merge solution (if we want another option beyond _.extend). That option could also potentially allow developers to omit the nested object keys. e.g.

options = Merge.merge( { defaults }, options );
// or
options = new Merge( defaults, options );
///////
options.getFlat() // nested keys omitted
/* or something like */
noSubOptions = Merge.removeNestedOptions( options );

[pixelzoom]

To evaluate the original example in #91 (comment), we should see what it looks like with our current _.extend pattern, so see below. In general, the default values for each set of nested options are filled in with an additional _.extend call. I haven't had the need to nest more than 1 level deep yet, but I suppose that could be encountered.

function MyNodeTypeWithHSliderInIt( options ) {

    options = _.merge( {
      visible: false,
      pickable: false,
      hSliderOptions: null // nested options passed to HSlider, defaults set below
    }, options );

    options.hSliderOptions = _.extend( {
      endDrag: function() { ... }, 
      startDrag: function() { ... }
    }, options.hSliderOptions,  );
  
    var slider = new HSlider( new Property(), new Range(), options.hsliderOptions );
  }

[jonathanolson]

I'll definitely clarify any potential objections during the dev meeting, but it seems workable to only recurse into keys that end in Options. The example would be:

constructor( options ) {

  options = merge( {
    visible: false,
    pickable: false,
    hSliderOptions: {
      startDrag: () => {},
      endDrag: () => {}
    }
  }, options );

  const slider = new HSlider( new Property(), new Range(), options.hsliderOptions );
}

where we would use our own import (PHET_CORE/merge), and the nested options would not need to be separated out.

[mbarlow12]

Another element to consider is that when nested options are used in a base type, the pattern @pixelzoom illustrated in #91 (comment) has to be propagated to subtypes and potentially suptype instantiations. For example, a sim may create a type that uses MyNodeTypeWithHSliderInIt

function MyNodeThatNeedsNestedOptions( options ) {

    options = _.merge( {
      visible: false,
      pickable: false,
      nodeWithHSliderInItOptions: null
    }, options );

    var nodeWithHSliderInItOptions = _.extend( {
      hSliderOptions: null,
      maxWidth: 44,
      ...
    }, options.nodeWithHSliderInItOptions );
    
    nodeWithHSliderInItOptions.hSliderOptions = _.extend( {
      endDrag: function() { ... }, 
      startDrag: function() { ... }
    }, nodeWithHSliderInItOptions.hSliderOptions );
  
    var nodeWithHSliderInIt = new MyNodeTypeWithHSliderInIt( new Property(), new Range(), nodeWithHSliderInItOptions );
  }

this also doesn't show the issue where multiple instances of MyNodeThatNeedsNestedOptions each have different start/end drag functionalities which would require additional extending at instantiation.

[pixelzoom]

2/14/19 dev meeting consensus:

• @mbarlow12 will implement
• Recurse only into keys with Options suffix
• assert that the the value doesn't have prototype.

[mbarlow12]

I've added the merge function and a large number of unit tests, but it's possible I've missed some cases to check.

Thus far, it seems to be working well but explicitly checking the key for 'Options' as well as testing that it's prototype is the same as Object. It will also throw an assertion error if the ...Options key holds anything other than a brace-style object (e.g. someNestedOptions: {...}). Thus, if we wanted to set the nested options with a function

someNestedOptions: function() {
  return {
    key: 'value'
  };
}

that will currently fail, too.

One departure from _.merge() is that I'm not handling/merging arrays that contain objects. They are currently treated as any other key. I don't recall exactly how frequently arrays appear in our options, but the following scenario is a little tricky:

const o1 = {
  subOptions: {
    arr: [
      {
        moreOptions: {
          key1: 'val',
          key2: 'val2'
        }
      },
      { key4: 'val4' }
    ]
  }
};

const o2 = {
  subOptions: {
    arr: [
      {
        moreOptions: {
          key1: 'new val',
          key3: 'another'
        }
      },
      { key3: 'value3' }
    ]
  }
};

const o3 = merge( o1, o2 );

lodash's implementation would result in

o3 = {
  subOptions: {
    arr: [
      {
        moreOptions: {
          key1: 'new val', 
          key2: 'val2', 
          key3: 'another'
        }
      },
      {
        key4: 'val4',
        key3: 'value3'
      }
    ]
  }
}

and is dependent on the order in the array. It seems safer to me to treat arrays as other values. If a developer needs combine arrays within options, I think that should be handled outside of merge/extend.

[samreid]

I have case in Wave Interference where it would be appropriate to use merge (I think), however, it seems like merge is mutating the arguments. It's unclear whether this is desirable.

For instance:

    const a = {
      sliderOptions: {
        hello: 'there'
      }
    };
    const b = {
      sliderOptions: {
        time: 'now'
      }
    };
    merge( {}, a, b );

results in

JSON.stringify(a,null,2)
"{
  "sliderOptions": {
    "hello": "there",
    "time": "now"
  }
}"

Is that as expected? What I'm seeing in Wave Interference is that "units: 'nanometers'" is getting injected into NUMBER_CONTROL_OPTIONS and used in places that shouldn't have "units: 'nanometers'".

I'm not sure whether I should tinker with merge or do something simulation-specific until we discuss/address this.

[pixelzoom]

merge is clearly modifying the value of its obj argument:

  function merge( obj ) {
     ...
    return obj;
  }

That violates best practices for options (see #96):

• Do not mutate the options argument value. Always assume that the value you are passed might be used elsewhere.

... and is likely why NUMBER_CONTROL_OPTIONS (which is presumably intended to be a constant) is being modified.

[pixelzoom]

On the other hand, maybe merge is intended to work like _.extend, and modify it's first arg. Hard to tell without documentation. But if that's the case, then @samreid's workaround in phetsims/wave-interference@3f3d173 is actually the correct way to do this.

const merged = merge( {}, options, NUMBER_CONTROL_OPTIONS );

[pixelzoom]

Hmmm... @samreid originally had something like:

merge( {...}, NUMBER_CONTROL_OPTIONS ) );

... and reported that NUMBER_CONTROL_OPTIONS was being modified. Do I have that right? If so, there are bigger problems here.

[pixelzoom]

Are the unit tests in mergeTests.js adequate? Do they verify that objects that should not be modified are unchanged after a merge?

[samreid]

Do I have that right? If so, there are bigger problems here.

I had this code:

      super( title, property, property.range, merge( {
        sliderOptions: {
          majorTicks: [ {

            // TODO: model coordinates for these
            value: property.range.min,
            label: new WaveInterferenceText( minLabel )
          }, {
            value: property.range.max,
            label: new WaveInterferenceText( maxLabel )
          } ]
        }
      }, NUMBER_CONTROL_OPTIONS, options ) );

And it was modifying NUMBER_CONTROL_OPTIONS. If I switch the order to:

}, options, NUMBER_CONTROL_OPTIONS ) );

Then it works correctly, but options gets mutated (and order of application/precedence is unclear).

[samreid]

One more point for code review. We discussed merging for keys that end with Options, but the implementation looks like it is instead checking prop.includes( 'Options' )

[pixelzoom]

Documentation for merge.js does not pass code review. I've add TODOs to the code.

[pixelzoom]

All args expect the last one are modified because merge is calling itself recursively:

31 Object.defineProperty( obj, prop, merge( obj[ prop ], source[ prop ] ) );

This is a problem. And yes, it means the unit tests are inadequate. Recommended not to use merge until this is fixed.

[pixelzoom]

Hmm... Or again, is this intended behavior? Hard to tell without documentation. I'm not going to do any more work on this until @mbarlow12 has had a chance to chime in.

[pixelzoom]

I expected behavior like ._extend, where only the first arg is modified. E.g.:

> var a = { a: 1, b: 2 }
> var b = { c: 3, d: 4 }
> _.extend( a, b, { e: 5, f: 6 } )
> a
{a: 1, b: 2, c: 3, d: 4, e: 5, f: 6 }
> b
{c: 3, d: 4}

[samreid]

@zepumph volunteered to review merge and its usages and unit tests. That will put us in a better position to understand what the next steps are.

[samreid]

Also, in review @chrisklus and @zepumph and I looked at code with merge and without merge and agreed merge is so much nicer it is worth the trouble to get it into the mainstream.

[zepumph]

From the review:

• Should we allow this case?

options = _.extend( {
  xOptions: null // filled in below
  . . . 
}, options );
options =  merge( { 
  xOptions: { test: 1 } 
}, options );

Since xOptions, is null it fails the type check validation. I ran into this in a pattern in INVERSE_SQUARE_LAW_COMMON/ISLCObjectNode and NumberControl. Where it is nice merge after an extend call so the merge can use some of the parent options. I think we may want to either set to an empty object, or just omit this nested options object from the original extension.

• // TODO phet-info#91 by calling recursively, this mutates all except the last arg. Should only mutate the first arg, ala _.merge and _.extend.

  I could not find any spot where we were mutating anything except the final target (first arg). There are also a few tests confirming this. I'm going to remove this todo.

UPDATE: I just had the idea to add Object.freeze calls to some sources in the tests to make sure they aren't being set. That boosts my confidence levels for this one.

• Currently merge supports a nested options field called Options: {} with no prefix. Should we support this?

Review is complete. It would be nice to discuss these points out with someone, though I'm not really sure who. We could discuss at developer meeting but that seems more costly than needed. @ariel-phet will you please assign someone to look over my review before bringing this to developer meeting.

[zepumph]

In the above commit I added support for optional options objects to be passed as sources. I also added tests for that. Basically it works by ignoring any source that is undefined, but it would assert out if a source was {null|string|number|etc}.

[zepumph]

From developer meeting today...

Should we allow this case?

• No we don't allow that case, *Options values must be objects.

How to handle options used in other options:

• Have an extend call first which merge all options used in other options, and then use those in the below merge.

  // options used in options set below. Using extends because it is a flat list, but perhaps it is better to use `merge` in this case.
  options = _.extends( {
        tickLabelFont: new PhetFont( 12 )
      }, options );

   options = merge( {
      sliderOptions: {
        majorTicks: [
          { value: range.min, label: new Text( range.min, { font: options.tickLabelFont } ) },
          { value: range.max, label: new Text( range.max, { font: options.tickLabelFont } ) }
        ]
      }
  }, options );

• rename: mergeOptions, mergePhetOptions, phetMerge. Something that should be clear that merge is only recursive for '*Options' keys. Straw poll indicates merge is the winner for now, especially because we may support *Config recursion.

• Support *Config recursion? For now let's not. Currently the developers aren't yet totally settled on the config pattern in general. Let's wait for the "best practices" talk to happen.

• replace all usages of _.extends with our merge library? We considered this, and think it should probably be done at some point in the future. It will decrease the cognitive load of options to only have one way of doing things, instead of two, similar but different ways to be used in different cases. Not right now though. Let's wait until we have used it more thoroughly and our merge library very very vetted. Are there performance considerations?

Currently merge supports a nested options field called Options: {} with no prefix. Should we support this?

• Options as a key is not allowed. Make that error out.

• assert that option keys are named with a lowerCase? No need, thanks though.

• Let's write more tests that demonstrate the above changes, making sure we understand the feature set.

Thanks all! I will implement these, and then mark for dev meeting again as a "ready for mass consumption" PSA.

[zepumph]

Alright the above has been implemented. As per the discussion in developer meeting from August 8th, merge is ready for mass consumption. Marking on developer meeting as a PSA. Hopefully we can discuss how this function plays into our options strategy as part of the options/config design pattern discussion coming up.

[Denz1994]

Discussed at dev meeting: 09/19/19

Merge() is fully operational and we can decide how to use this in the next software design pattern meeting.

Assigning to @jbphet to read through and close.

[jbphet]

Thanks. I've read through and ingested the information, and we can finalize how PhET uses merge in the next Software Design Pattern meeting (as @Denz1994 notes above). Closing.