A better way to pass through configuration

[samreid]

While working on adding nested options to NumberControl, @pixelzoom said:

I still think there must be a better approach (than options) to customizing UI components.

I want to take a look at an idea I had for this.

[samreid]

The main idea: each object knows about its immediate children and provides defaults for them. If you want to provide customized structure for nested object, the client creates it. This eliminates the need for creating and mapping a multitude of variable names. For example:

function House( options ) {
  options = _.extend( {
    frontDoor: new FrontDoor(), // default front door
    roof: new Roof(), // default roof
    floor: new Floor() // default floor
  }, options );
}
function Roof( color ) {
  this.color = color || 'gray';
}
function Floor() {}
function FrontDoor() {}
function Doorknob( clockwise ) {
  this.clockwise = clockwise;
}

var myHouse = new House( {
  roof: new Roof( 'blue' ), // roof is default except for the color
  frontDoor: new FrontDoor( {  // front door is default except for the doorknob
    doorknob: new Doorknob( true ) // doorknob is default except for the clockwise
  } )
  // floor is default
} );

[samreid]

In the above example, the House constructor doesn't need to know anything about the substructure of Door (for instance, that it has a doorknob). If the client wants defaults, it does nothing. If it wants to override something, it has full control over how the overriding is done.

Con: in the worst case scenario, the scene graph gets constructed twice, once for options and once with its replacement. But I bet we can come up with a pattern that obviates this.

[samreid]

Here's a mock-up of our existing pattern, that we don't like too much:

function House( options ) {
  options = _.extend( {
    frontDoorDoorknobClockwise: true,
    roofColor: 'gray',
    floorTexture: 'carpet',
  }, options );
  this.frontDoor = new FrontDoor({doorknob: new Doorknob(options.frontDoorDoorknobClockwise)})
  this.roof = new Roof(options.roofColor);
  //etc.
}
function Roof( color ) {
  this.color = color || 'gray';
}
function Floor() {}
function FrontDoor() {}
function Doorknob( clockwise ) {
  this.clockwise = clockwise;
}

var myHouse = new House( {
  frontDoorDoorknobClockwise: false,
  roofColor: 'blue'
  // floor is default
} );

[samreid]

Some thoughts about NumberControl:

  function NumberControl2( options ) {

    // Don't double create everything, nice!
    var titleNode = options.titleNode || new Node(); // Any kind of node can be used for the title, nice!
    var slider = options.slider || new HSlider();
    var leftArrowButton = options.leftArrowButton || new ArrowButton( 'left', function() {}, {} );

    // Con: Some things are likely duplicated with how client would create them.
    var rightArrowButton = options.rightArrowButton || new ArrowButton( 'right', function() {}, {} );

    // For instance, the same property should be specified in HSlider and NumberDisplay.  Can we "curry" it out, or is
    // it OK that it must appear multiple places?

    // Or what if you can do Component.setProperty() and it would use the new property?  That would open up flexibility for this pattern.
    var numberDisplay = options.numberDisplay || new NumberDisplay();

    Node.call( this, { children: [ titleNode, slider, leftArrowButton, rightArrowButton, numberDisplay ] } );
  }

How to make it easy for all subcomponents to use the same Property? Perhaps add setProperty to them, or let them have their own properties, and bind them using a two-way binding.

EDIT: Or curry it out, by providing a function that takes a Property and returns a Component. Then instead of passing in a Component instance, the client would pass in a function that takes everything except the Property, and the component supplies the Property.

[samreid]

I fleshed out a NumberControl2 prototype and example usage. This pattern seems promising, some thoughts:

1. Composite components such as NumberControl should not have to know all about subcomponents structure and options, and sub-subcomponents structure and options, etc. It should only know about its direct descendants and its own parameters. This pattern captures that pattern nicely.
2. This pattern avoids extraneous allocations with _.extend(), by only creating things when they are needed.
3. This pattern is much more flexible. For instance, I replaced an ArrowButton with a TextPushButton in NumberControl2 and everything worked perfectly. This did not required adding extra options or passing them through.
4. I was worried about having to pass the same property to multiple places, but in practice this seemed acceptable, and again opens up flexibility.
5. Even if this is a great pattern, it would require a herculean effort to port pre-existing components to use this pattern because it entails a significant API change. That being said, I'd be interested to see what a full stack of components and usage sites would look like if we pursued this pattern. And, it should be considered for future development.

[samreid]

I'd like to discuss this pattern with @pixelzoom before having a larger group discussion. @pixelzoom let me know your thoughts at your convenience, or if you would like to voice chat about it. The changes above are committed to scenery-phet/configuration branch and you can test it by running http://localhost/scenery-phet/scenery-phet_en.html?screens=2

[samreid]

The example usage is given in SlidersView.js:

    // NumberControl with default layout
    var numberControl1 = new NumberControl2( 'Weight:', weightProperty, weightRange, numberControlOptions );

    // A customized one
    var numberControl2 = new NumberControl2( null, weightProperty, weightRange, {
      titleNode: new Text( 'HELLO THERE', {
        fontSize: 20
      } ),
      slider: new HSlider( weightProperty, weightRange, { thumbFillEnabled: 'green' } ),
      leftArrowButton: new TextPushButton( 'REDUCE', {
        listener: function() {
          weightProperty.value = Math.max( weightProperty.value - 10, weightRange.min );
        }
      } )
    } );

And I haven't made any effort to implement various layouts. It should look like this:

[pixelzoom]

My quick evaluation...

Pros:

1. Component may have a smaller default options hash
2. Component doesn't need to propagate/forward option values to its subcomponent
3. Coarser grained customization: ability to use different types of subcomponents, as long as they implement a specified interface.

Cons:

1. Weak/leaky encapsulation: Exposes the inner workings of the component - how many subcomponents it has, the (duck?) type of those subcomponents, etc.
2. Substantial increase in the documentation required to describe the (duck?) type of subcomponents. We'll be relying more heavily on interfaces in a language (JS) that doesn't support them.
3. Substantial increase in validation code required to verify that subcomponents provided by clients comply with required interfaces.
4. Increased need for consistency of names across things that might be used as subcomponents. (Consistency is good, but now essential if relying duck typing.)
5. If a client wants to change even 1 option default (which happens frequently and it the whole point of providing options) then the client will need to create its own subcomponent(s). That's a substantially heavier responsibility for the client, and has the effect of locating more implementation details at the client call site.
6. If "coarse grain customization" is used (using different subcomponent types), it may become difficult to identify the concrete types that are used as subcomponents for a component.
7. Cons 1,5,6 will make it substantially more difficult to change the implementation of a component, since they leak implementation details to client sites.

[samreid]

Are we facing the problem of dependency injection?

[samreid]

Yes, it appears this is the same problem as dependency injection, and the standard solution for this problem is an inversion of control container. The simplest form of this is to provide a service or function that, when called, returns an appropriate value. I'm trying this for NumberControl and it seems promising.

[samreid]

I'm testing out:

      createTitleNode: function( title, options ) {
        return new TandemText( title, options );
      },

But we could alternatively use:

      TitleType: TandemText

I'm experimenting with the former first because it has direct lookups instead of dynamic names.

[samreid]

Terminology for below: "Current approach", or "Approach 1": providing adapter options with name prefixes like so (example is from NumberControl):

      // value
      valueFont: new PhetFont( 12 ),
      valueMaxWidth: null, // {null|string} maxWidth to use for value display, to constrain width for i18n
      valueXMargin: 8,
      valueYMargin: 2,
      valueBackgroundStroke: 'lightGray',
      valueBackgroundLineWidth: 1,
      decimalPlaces: 0,

then remapping the names when passed to the subcomponent:

    var numberDisplay = new NumberDisplay( numberProperty, numberRange, {
      valuePattern: options.valuePattern,
      font: options.valueFont,
      decimalPlaces: options.decimalPlaces,
      xMargin: options.valueXMargin,
      yMargin: options.valueYMargin,
      backgroundStroke: options.valueBackgroundStroke,
      backgroundLineWidth: options.valueBackgroundLineWidth,
      maxWidth: options.valueMaxWidth,
      tandem: options.tandem && options.tandem.createTandem( 'numberDisplay' )
    } );

This approach has the disadvantage that for each of N subcomponents, we must provide M adapter options, and we are likely to forget some, or for them to get out of sync, collide, etc. Providing options for grandchild components becomes even more of a mess, and we have pretty much avoided this due to the hassle.

Approach 2 This was proposed in https://github.com/phetsims/tasks/issues/730#issuecomment-260541475 and clients provide components in the options hash.

Approach 3 Gives more flexibility to the component classes by using functions options instead of component options. The API for a default one remains the same:

    // NumberControl with default layout
    var numberControl1 = new NumberControl3( 'Weight:', weightProperty, weightRange, numberControlOptions );

The implementing component provides options that create its immediate children (not its nested sub sub children).

function NumberControl3( title, numberProperty, numberRange, options ) {
    options = _.extend( {

      // Functions that create sub-components
      createTitleNode: function( title, options ) {
        return new Text( title, options );
      },

      // etc...

      // Options specific to this component itself (not about its sub-components)
      subComponentSpacing: 10

    }, options );

So it can be customized like so:

    // A customized one
    var numberControl2 = new NumberControl3( 'Big title', weightProperty, weightRange, {
      createTitleNode: function( title, options ) {
        return new Text( title, _.extend( { fontSize: 32 }, options ) );
      }
    } );

Hence we attain the full flexibility of being able to customize the title node in any way we desire (using the full Text interface) without providing adapters for any of the options. Subcomponent options specified by the NumberControl3 can be overriden. We can even use a different subtype of subcomponent, if needed. The types are checked in the component class like so:

    var titleNode = options.createTitleNode( title, {
      font: new PhetFont( 12 ),
      maxWidth: null, // {null|string} maxWidth to use for title, to constrain width for i18n
      tandem: options.tandem && options.tandem.createTandem( 'titleNode' )
    } );
    assert && assert( titleNode instanceof Text );

This solution gets rid of the messy options in the current approach, which must be duplicated and renamed when passed to the components, and may be incomplete or out of sync, collide with other options, etc.

Here's an example showing a doubly-nested customization:

var numberControl2 = new NumberControl3( 'Big title', weightProperty, weightRange, {
  createNumberDisplay: function( numberProperty, numberRange, options ) {

    // Note the reversed order of extend, so that we overwrite the defaults specified in NumberControl3
    return new NumberDisplay( numberProperty, numberRange, _.extend( options, {
      createReadoutNode: function( title, options ) {
        return new Text( title, _.extend( options, {
          fill: 'blue'
        } ) );
      },
      backgroundStroke: 'black'
    } ) );
  }
} );

A full example is available in the "configuration" branch of scenery-phet, please see NumberControl3 and the sample usage in SlidersView.

I'll discuss the proposed Approach 3 in terms of the pros and cons mentioned for Approach 2 in https://github.com/phetsims/tasks/issues/730#issuecomment-260802858

Pros:

1) Component may have a smaller default options hash

Approach 3 makes the main options the types of the direct subcomponents (not sub-subcomponents) and options for the direct subcomponents would be specified elsewhere. See NumberControl3 for an example.

2) Component doesn't need to propagate/forward option values to its subcomponent

Approach 3 has this advantage as well.

1. Coarser grained customization: ability to use different types of subcomponents, as long as they implement a specified interface.

Approach 3 has this advantage as well.

Cons:

1) Weak/leaky encapsulation: Exposes the inner workings of the component - how many subcomponents it has, the (duck?) type of those subcomponents, etc.

The current approach "leaks abstraction" by providing a multitude of subcomponent-specific options. On the other hand, Approach 3 leaks abstraction by identifying the types of the subcomponents. In my opinion, the latter leak is preferable to the former. For example: a NumberControl could expose 8+"NumberDisplay-esque" options which get renamed and forwarded over to NumberDisplay, or it could simply say that it contains an NumberDisplay.

2) Substantial increase in the documentation required to describe the (duck?) type of subcomponents. We'll be relying more heavily on interfaces in a language (JS) that doesn't support them.

This is addressed in Approach 2 by documenting the default type and then checking that the provided type is instanceof the required type. See for example: https://github.com/phetsims/scenery-phet/blob/dd3f5c5e1b067637b0ba063ed5ee5044b3107d63/js/NumberControl3.js#L112-L112

3) Substantial increase in validation code required to verify that subcomponents provided by clients comply with required interfaces.

The solution for this is described in (2)

4) Increased need for consistency of names across things that might be used as subcomponents. (Consistency is good, but now essential if relying duck typing.)

Naming consistency will help us during development. For instance MyCustomHSlider could be substituted for HSlider, and would be a better name than MyCustomWidgetDevice. But we should be striving for consistency of names anyways (even if we don't opt for Approach 3).

5) If a client wants to change even 1 option default (which happens frequently and it the whole point of providing options) then the client will need to create its own subcomponent(s). That's a substantially heavier responsibility for the client, and has the effect of locating more implementation details at the client call site.

This is the primary drawback of Approach 3, to be weighed against its "pros".

6) If "coarse grain customization" is used (using different subcomponent types), it may become difficult to identify the concrete types that are used as subcomponents for a component.

Yes, if I provide a MyCustomHSlider instead of an HSlider to a NumberControl, that could be harder to read in the code. If we decide to hard code the types, we could just provide hooks for overriding the options that go to each component. But we should discuss the flexibility/complexity tradeoff here.

7) Cons 1,5,6 will make it substantially more difficult to change the implementation of a component, since they leak implementation details to client sites.

Yes, this is somewhat a disadvantage, but not altogether different than the current approach. In the current approach, if I change from NumberDisplay to some other type for showing the numbers, then I would probably have to update many of the 8 options related to it as well. (Leaking through 8 options instead of 1 type).

The main point of Approach 3 is that a component should only know about its own children and options, not about its grandchildren or beyond, and that clients using a component will be able to fully customize any descendant if desired without any adapter options and remappings.

@pixelzoom I'm interested in discussing this with you further when you have time.

[samreid]

Approach 4 Assume the types are hard-coded and the only customization comes from providing different options to the sub-components (and nested sub-sub-components). The usage might look something like this:

    // A customized one
    var numberControl2 = new NumberControl3( 'Big title', weightProperty, weightRange, {
      titleNodeOptions: { fontSize: 32 },
      numberDisplayOptions: {
        readoutNodeOptions: { fill: 'blue' },
        backgroundStroke: 'black'
      },
      sliderOptions: { thumbFillEnabled: 'green' },
      arrowButtonOptions: {
        arrowStroke: 'blue',
        arrowLineWidth: 2
      }
    } );

Note the doubly-nested readoutNodeOptions. I think we discussed this before, but not sure why we ruled it out. This seems less powerful than Approach 3 but also much simpler for the client.

[samreid]

Reassigned to myself to look into Approach 4 further before discussion.

Edit: Approach 4 with the option to augment with Approach 3 in cases where we explicitly want to support subcomponent subtyping or more flexible construction.

[samreid]

I'm interested in investigating Approach 4, but I do not have time at the moment. And even if I did have time and we decided we wanted to move forward with Approach 4, it would be quite a bit of work to redesign old components to use this approach. My recommendation: evaluate this strategy the next time we make a composite UI component.

[pixelzoom]

I agree that we should revisit Approach 4 (https://github.com/phetsims/tasks/issues/730#issuecomment-260863644), using an _.extend call for each of the nested options hashes.

[samreid]

It's difficult to imagine retrofitting old APIs with Approach 3 or 4 (unless they have minimal usage or need major surgery), but perhaps we should keep those ideas in mind as we develop new common code components (which we are doing with notably less frequency these days). Let's close for now, this closed issue can be a reference for what to do next time we have a common code component.

[pixelzoom]

... perhaps we should keep those ideas in mind as we develop new common code components (which we are doing with notably less frequency these days)

I doubt that anyone will keep these ideas in mind if the issue is closed - we're more likely to continue to use flat options and propagate the entire options hash. Vegas is about to get a bunch of new components. And I suspect that common components in general will see significant revisions as we continue to outfit for PhET-iO, a11y, sonification, etc -- which are all likely to involve more options. That said...

In unit-rates, I used nested options and it worked out nicely. From implementation-notes.md:

---

Nested options: In this simulation, I tried a new pattern for nesting options. It allows clients to specify only the nested options that they wish to override. The pattern is used throughout the sim, mostly for specifying options related to a rate's numerator and denominator (e.g. in DoubleNumberLine). The general pattern is:

options = _.extend( {
  nestedOptions: null, // {*} to be filled in with defaults below
  ...
}, options );

options.nestedOptions = _.extend( {
  // default values go here
}, options.nestedOptions );

[samreid]

I doubt that anyone will keep these ideas in mind if the issue is closed

Should we label this for developer meeting to raise awareness?

[jbphet]

This issue is quite long, and was closed and then reopened with a comment about "raising awareness". I'm assuming that we will discuss and disseminate at the developer meeting and that further input on whether we should migrate old code to one of the new approaches is not desired at this point.

I will be developing new common code components in support of sonification, so it would be good to emerge from the meeting where this is discussed with a clear consensus about which of the approaches described above is the preferred approach for new common code.

[samreid]

The proposed approach is to pass options objects for sub-components as described in https://github.com/phetsims/tasks/issues/730#issuecomment-370860967.

After everyone has "signed off" on this issue, we'll close it and reference it from the code review guidelines.

[samreid]

Assigned to @jbphet to review the preceding comment and add a reference to this to the code review guidelines. Let us know if you have any questions.

@pixelzoom points out an exception to the proposed strategy is where we want to hide the implementation details of how the component uses a sub-component--in that case it may make sense to use new top-level options.

[jbphet]

I've reviewed the comment and added a checklist item to the code review checklist, which is what I think @samreid was suggesting in the comment just above. @samreid - please review and close, edit, or reassign back to me at your discretion.

[samreid]

Looks great, thanks! Closing for now. Once reviewers are using the new documentation/pattern, they may have recommended changes--in that case we can reopen this issue and discuss further.