[WIP] New Interface

[rawls238]

@eytan @gusvargas

This is effectively a total rewrite of this library, hopefully making it smaller in size, easier for end users to use it for more complicated experiments, develop their own custom experiment components around the parametrize "primitive" component, but still be able to use it for more simple use-cases (using the same underlying engine).

This implements the following:

<Parametrize experimentClass={exp} {...arbitraryProps}>
     <MyComponent />
</Parametrize>

where Parametrize passes all the experiment parameters from the experiment class down to its immediate children using props and to all its descendants using context. There are examples for using this in the example index.html.

Using this, I re-implemented the Experiment and Variation components to effectively be convenience wrappers around the parametrization component, but make them more powerful than they previously were.

The Experiment / Variation components can be used in the following ways:

<Experiment experimentClass={foo}>
      <MyComponent />
</Experiment>

<Experiment experimentClass={foo} param='bar'>
   <Variation name='charlie'>
        <MyParametrizedComponent  /> //this can use all the experiment parameters associated with foo
   </Variation>
   <Variation name='foobar'>
        <MyOtherParametrized Component /> 
  </Variation>
  <Default>
      foo
   </Default>
</Experiment>

<Experiment experimentClass={foo}>
   <Variation name='foo' param='foobar'>
      <Variation name='foobar' param='foobarz'>
        hey 1
      </Variation
  </Variation>
   <Variation name='foo' param='foobar'>
      <Variation name='foobarz' param='foobarz'>
        hey 2
      </Variation
  </Variation>
</Experiment>

Effectively this allows nested variation components when needed as well as the ability to parametrize any nested children components.

The design of this is effectively based on the underlying assumption that experimental parameters are either: 1) Parameter values 2) Branching logic (i.e. testing between two completely different UIs or something)

Feel free to pull down this branch locally and toy with the examples in the index.html. I want to write some tests, but wanted some eyes on this first.

[rawls238]

also note that this is probably a better view of the diff:

https://github.com/HubSpot/react-experiments/tree/guy_playground

[rawls238]

@eytan @gusvargas @mattrheault this should be good to go, comments appreciated especially on the readme.

[gusvargas]

I haven't look at the code yet but at a high level I'm not sure that passing exp params to immediate children as props and deeper childen as context is a good idea. I think ideally we should be consistent and allow any arbitrarily nested component to access params through context.

What we could do is leverage high-order components to abstract away the context stuff and provide the params as props. For example:

// Button.js
const Button = React.createClass({
  handleClick() {
    console.log(this.props.someOtherPropNotInTheExpParams);
  },

  render() {
    return (
      <button style={{ color: this.props.color }} onClick={this.handleClick}>
        {this.props.text}
      </button>
    );
  }
});

export default WithExperimentParams({
  color: React.PropTypes.string,
  text: React.PropTypes.string
})(Button); // Returns a component

// App.js
export default React.createClass({
  render() {
    return (
      <Parameterize experiment={MyButtonExperiment}>
        <Button someOtherPropNotInTheExpParams='foo' />
      </Parameterize>
    );
  };
});

Where WithExperimentParams does something like:

export default (experimentParams) => {
  return (Component) => {
    return React.createClass({
      contextTypes: experimentParams,
      render() {
        return (
          <Component {...this.props} {...this.context} />
        );
      }
    });
  };
};

This way the consumer doesn't have to wire up context manually and read from it. This is similar to what redux does.

Also, I'm not sure that names Experiment and Variation actually convey what it is that they're doing. They are essentially component syntactic sugar for doing a branch off of one param. I hate to argue semantics but maybe something like:

// Assuming `MyUIOverhaulExperiment` has 4 params
// uiType, buttonColor, buttonText, numberOfItems
// uiType is a choose between ['control', 'with-dragons', 'with-cats']
<Branch on='uiType' experiment={MyUIOverhaulExperiment}>
  <When param='control'>
    <TheOldUI />
  </When>
  <When param='with-dragons'>
    <TheUIWithDragonGifsEverywhere />
  </When>
  <When param='with-cats'>
    <TheUIWithCatGifsEverywhere />
  </When>
</Branch>

// TheUIWithDragonGifsEverywhere.js
export default WithExperimentParams({
  buttonColor: React.PropTypes.string,
  buttonText: React.PropTypes.string
  numOfItems: React.PropTypes.number
})(React.createClass({ // you get it. }));

[rawls238]

I think the higher-order component idea is fantastic.

We discussed this offline, but came to the agreement that we should have the top-level component remain the Experiment component but change param -> on and change the Variation component from

<Variation param='bar' name='foo'>

to

<When param ='bar' value='foo'>
```javascript

[eytan]

Hi @rawls238 -- this is a huge improvement over the prior version! I agree with @gusvargas that it seems bad to pass any property to anything other than the immediate children, and I like the higher-order approach because (1) it simplifies the code and makes it easier to read (2) makes it really explicit what parameters you are experimenting with, and what their types are.

A few thoughts:

(1) I might be a little primed by the branching example above (and am reminded of SQL's CASE/WHEN syntax), but I find <When> confusing because the name seems to indicate some type of mutual exclusion, but if one specifies both a param name and its value to be matched, the cases need not be mutually exclusive.

For example, in the case of 2x2 factorial design:

  foo <- bernoulliTrial(p=0.5, unit=cookeid);
  bar <- bernoulliTrial(p=0.5, unit=cookieid);

the block

...
<When param ='foo' value='1'>
  <p>Sign up</p>
</When>
<When param='bar' value='1'>
  <p>Create account</p>
</When>
...

Would potentially render both sign up and create account... or alternatively if we were to change the ordering of the variants, it would break the expected behavior. Because of this, I would favor something similar to @gusvargas's suggestion, where you have a parent component that specifies which param you are switching on, and all children are mutually exclusive by construction.

(2) It may end up that A/B testing completely separate components may not be a very common use case in many situations. If you already have a component that takes a number of parameters (props), why not just build that branching into the component itself? (perhaps this is part of the offline discussion where you decided on not having a component called Branch :).

One possible solution would be to have two different different components reflecting these styles of experimentation: <ABTest experiment=blah param=blah><When value=blargh>...</ABtest>

which might be sugar for something like: <PlanOut experiment=blah><Branch param=blah><When value=...>....

and <PlanOut experiment=blah><MyComponent/></PlanOut> for more generic experiments where you are not explicitly A/B testing a few variants.

(3) If you are baking the experimentation into the component definition, why do you need the <Experiment experiment={name}> component wrapper at all? You could then just have WithExperimentParams be ParameterizeComponent(experimentName,propTypes)(component).

(4) My understanding is that your PlanOut parameter names have to be the same as your prop names. This seems like a pretty good idea, and I would clarify this in the docs/README.

[gusvargas]

Hey @eytan,

Thanks a lot for all for all your feedback thus far. Using some Parameterize functionality as a primitive ultimately ended up making much more sense than our first pass.

Re: Thought 1 @rawls238 and I spoke offline once more after his most recent comment and we came to an agreement that we would only allow branching on a single param defined by the parent component (ABTest, Branch, or whatever), i.e., Whens would be mutually exclusive.

Re: Thought 2, 3 At HubSpot, experimenting and essentially "branching" off one parameter is and has been a fairly common case. Sometimes we introduce drastically different UIs and PlanOut allows us to perform rollouts incrementally in the form of an experiment. We can then use the exposure logging to verify that downstream events maintain similar conversion rates to the control. Because of this, we wanted to provide some component-based sugar that abstracts away the explicit branching in code while providing a visual mechanism in JSX that quickly describes the experiment. That said, we're aware that the star of the show is actually WithExperimentParams (or whatever we end up calling it). We still think there is some value in providing component based abstractions for ABTests as helpers even if just for readability sake. I think that @rawls238 and @mattrheault (we work on the same team) can agree that seeing an experiment declared in JSX in a parent component has some semantic value to it. Perhaps the idea of not including component abstractions like ABTest in this project is worth discussing.

Also, using this "branch sugar" approach had led to a slightly simpler productization process once a winner is declared, i.e., we can just remove the wrapping ABTest component from the parent along with the losers and delete the losing modules. The alternative being having to remove a intermediary "branching" component and raise the winner up a level. To be clear:

return (
  <div>
    <ABTest experiment={MyExp} param='ui-type'>
      <When value='old'>
        <OldUI />
      </When>
      <When value='new'>
        <OldUIWithCoolStuff />
      </When>
    </ABTest>
    <div className='some-unrelated-stuff'>
      // snip
    </div>
  </div>
);

becomes

return (
  <div>
    <OldUIWithCoolStuff />
    <div className='some-unrelated-stuff'>
      // snip
    </div>
  </div>
);

I think it's important to set the expectation that whatever branching component we provide (if any) is a just a helper built on top of some higher-order component function.

Thanks again for your interest in the project. Let me know what you think.

[eytan]

Awesome, thanks for the context @gusvargas! I think we are on the same page --- the branching component sugar is clear, easy to adapt, and covers a lot of use cases. I am really happy to see how this <Parameterize> idea has played out, and suspect that over time you'll find more use for multi-factorial designs. Looking forward to seeing how the project evolves...

[rawls238]

yea we really have liked the branching component that we have now since for the experiments that @gusvargas talks about, it is really easy to see what's going on and easy clean up the code after.

With regards to the higher-order components and type-checking, I like the idea of having the WithExperimentParams component parametrize children components but for now leaving the Parametrize component instead of also turning that into a higher-order component. Due to the implementation details, I don't think it would be simple to pass in the propTypes to WithExperimentParams but I think this is OK since we should anyways eventually defer type-checking to PlanOut.js.

Re: 4, yes I think that it should the planout parameter names should map to the parametrized prop names when possible. I'll add something about it to the README.

Also, I like the idea of renaming the Branch/Experiment component to ABTest, I'm going to change that as well.

Besides these minor changes at this point, I'm going to merge this in and release this tomorrow so we can start using this new interface. Let me know if you guys have any comments / suggestions about any of the implementation details or any additional test cases I should add.

[rawls238]

as far as I can tell this should be ready to go, feel free to PR any changes you guys find / file any issues for anything that seems wrong. I'm sure there are bound to be a few issues as this is a rather large change :).