A few ideas / thoughts

[eytan]

This is awesome! React and PlanOut always seemed like they could work nicely together, in that PlanOut randomizes parameter values and React components are parametric (via props).

A few ideas / comments:

(1) Experiment component: A React component's render() method should not modify state, but renderExposedVariation(), which gets called by render affects state. It also seems less than ideal to have to re-evaluate all of the logic in renderExposedVariation() every time the component needs to be re-rendered. Why not just save the component you want to be rendered to state in getInitialState() or componentWillMount()?

One thing to watch out for is that you want to make sure that your logging does not depend on your components successfully rendering -- otherwise you might not catch cases where your experiment is causing bugs, and you'll get non-equivalent users in your different experimental conditions.

(2) Parameters as props: part of the idea behind PlanOut is to get people thinking about experiments in terms of parameterizations of user experiences rather than variants. Such a notion seems compatible with the way one generally writes React code, in that components' behaviors or features depend on props.

You might do this generically by saying something like:

<Experiment experimentClass={foo}>
  ...
  <Parameterize propName1='param1', propName2='param2'>
    <MyComponent/>
  </Parameterize>
  ...
  <Parameterize propName3='param3'>
    ...
  </Parameterize>
  ...
</Experiment>

Where, for example, MyComponent is a component that includes two props, propName1 and propName2, which are populated with param1 and param2 from the experiment.

(3) Variations: I would suggest not marrying experiments to a single parameter, and instead having a <Variations> component that performs a function similar to what the existing Experiment component does, e.g.,

<Experiment experimentClass={foo}>
  <Variations variationParam='show_text'>
    <Variant name='experimental'>
      Example A
    </Variant>
    <Variant name='control'>
      Example B
    </Variant>
    <DefaultVariant>
      Things are broken.
    </DefaultVariant>
  </Variations>
</Experiment>

(4) Logging: If I understand correctly, you are only logging when there is a matching variant. This seems problematic...

(i) In the case of the logging example in the README, it's not clear what the treatment/control contrast is. Under the given scheme, it seems like you would likely only be logging / measuring effects on users who are not necessarily equivalent because those who hit different endpoints could be very different from one another.

(ii) Logging conditional on matching a known variant is also problematic when the components are causally downstream of one another. Let's imagine that you have some encouragement on page X that offered a discount (call this D=1), and we only expressed a variant to be shown when D=1 (so that only D is logged only when it equals 1), and then on page Y, the purchase form, we either shown the original price (D=0) or the discounted price (D=1). Visitors with D=0 are folks who would have visited Y regardless of the discount, while those with D=1 include marginal users who would not have otherwise visited (and may therefore be less likely to convert or spend as much). This means that even if you were to log all users who visit Y, those with D=0 and D=1 represent non-random populations. This is only an issue because you didn't exposure log all users who hit X because we were only logging users who had a matching variant at the time.

(5) Namespace component: Seems interesting, but doesn't seem practical from a code maintainability / readability perspective. I also wonder how this would play out as you add and remove experiments.

(6) Have you done much with Flux? I wonder how transportable this is to that kind of model.

Overall really neat stuff, I am looking forward to seeing how things progress with this!

[colbyr]

++ for (1)

In my mind, a better set up might look something like:

componentWillMount() {
  // setState in componentDidMount causes a rerender.
  // componentWillMount happens before the changes hit the DOM,
  // so if possible you should do your setStates there.
  this.selectVariation();
},

componentDidMount() {
  // componentDidMount fires one time *after* both componentWillMount and the component is in the
  // DOM. Consequently, it might be a better place to log exposure because once it runs you know the
  // (a) user has actually seen the component which is not necessarily the case in render and
  // (b) that `this.state.exposedVariation` is already set.
  this.props.experimentClass.logExposure();
},

render() {
  let variationComponent = ExperimentEnrollment.getExposedExperimentVariation(this.props.children, this.state.exposedVariation);
  return variationComponent;
}

That being said, there may well be some nuances of usage logging that I'm overlooking, in which case I'm interested to know more :wink:

[eytan]

If we move the variation selection and add the param transfer component the. The only purpose of experiment would be to pass down the experiment or namespace object to those components. Then, before this component renders its children, it should exposure log using the experiment / ns passed in as a prop, and save which component is selected via state. This way you know you know all users whose experiences could have plausibly be affected by the treatment are logged, even if some of the conditions cause the component's children to fail.

Sent from my iPhone

On Aug 21, 2015, at 5:34 AM, Colby Rabideau notifications@github.com wrote:

++ for (1)

In my mind, a better set up might look something like:

componentWillMount() { // setState in componentDidMount causes a rerender. // componentWillMount happens before the changes hit the DOM, // so if possible you should do your setStates there. this.selectVariation(); },

componentDidMount() { // componentDidMount fires one time after both componentWillMount and the component is in the // DOM. Consequently, it might be a better place to log exposure because once it runs you know the // (a) user has actually seen the component which is not necessarily the case in render and // (b) that this.state.exposedVariation is already set. this.props.experimentClass.logExposure(); },

render() { let variationComponent = ExperimentEnrollment.getExposedExperimentVariation(this.props.children, this.state.exposedVariation); return variationComponent; } That being said, there may well be some nuances of usage logging that I'm overlooking, in which case I'm interested to know more

— Reply to this email directly or view it on GitHub.

[colbyr]

@eytan ah I gotcha :eyes: To be clear, @rawls238 is expert/maintainer here, I only meant to contribute some lifecycle knowledge. I think what you're suggesting is excellent feedback.

[rawls238]

@eytan thanks a bunch for feedback - these are all great points and I've been meaning to write a response to this, but haven't had enough free time to sit down and think about some of these things. Here are some quick thoughts, I'll try to write a more in-depth response later and put up some PRs:

1) The original internal version of this library had this in componentWillMount and saved the component itself as state, but we ran into problems when we wanted the variation component to re-render when the parent component re-rendered. I think our current solution is less than ideal, but it should be an easy fix-up I think.

2 / 3) Agree with some of this but not 100% certain what the best path forward is. We definitely shouldn't marry experiments to single parameters, need to think of a clean way to generalize some of this stuff and I like your suggestions but want to play around with some other ideas. I'll attempt to write a more thought out response to this sometime soon.

4) Agreed, in my opinion the hardest part of this library is getting logging exactly right but still allowing a lot of flexibility for consumers of the library (it'll probably be necessary to set up some guardrails so that users don't botch experiments). The example I gave is definitely a bad example outside of the context I was thinking about it in :P. Feel free to PR any changes you think might help get this right.

5) Yea I also rather dislike the Namespace component. The first version of this library had very strict experiment component definitions so it was necessary at the time. I attempted to kill it when we made the experiment component definitions more flexible, but it still currently has its uses. Perhaps whatever we come up with regards to 2/3 will result in a cleaner API and we can kill the Namespace component (or make it better and more useful).

6) We use Flux with React, we haven't found a need to use Flux for anything around experiments. Also, I think it's important that if we add something to Flux, we have it be separate from the React library (or an optional add-on so that it's still usable in stacks that don't use Flux). Any ideas you had in particular around incorporating Flux?

[rawls238]

Actually regarding 5), the more I think about it the namespace component is actually useless at this point since you can just define two experiments next to each other using the same experimentClass and it would have the same behavior as wrapping the two in an Namespace class.

Quick example:

<Namespace experimentClass={foo}>
    <Experiment param="bar">
      <Variation name = "foobar" />
   </Experiment>
   <Experiment param="charlie">
    <Variation name="charlie" />
  </Experiment>
</Namespace>

is currently identical to

<Experiment experimentClass={foo} param="var">
  <Variation name = "foobar" />
</Experiment>
<Experiment experimentClass={foo} param="charlie">
    <Variation name="charlie" />
</Experiment>

[rawls238]

Few more notes around this issue, especially since I said I would leave a more thoughtful comment regarding changes to the primary interface:

1) The Namespace class should definitely go. Though its implementation can be useful, it's useless in its current state.

2) I don't like the idea of ALWAYS passing through props all the way down to the children components since this reduces the flexibility of the Variation components. However, I love the general idea of it as it seems as though it has the potential to be really powerful, especially for more complicated experiments.

I can't decide if it should be a standalone component or we should have it so that you can effectively "enable" parametrization by passing down a parametrize prop. It seems like it should be its own separate component since it represents completely different behavior vs the existing way of defining experiments.

However, I think it should be something as simple as this:

<Parametrize experiment={experiment}>
  <MyComponent test="test"/>
</Parametrize>

and all this does is take all the experiment parameters of the experiment Class (via getParams) and pass them through to MyComponent as props (in addition to any other props that are passed down to that component). This is cool since you can basically trivially define experiments over the props of a component.

3) I think we should just make things more flexible in the currently existing Experiment component for experiments that can't easily be defined in terms of parametrization. I have a strong opinion that we should keep a simple A/B test as simple as it currently is and have it so that the Variation component only has to be nested within two components instead of three. However, we should optionally allow a Parameter class to exist within an Experiment component and then have Variation components nested within there for multi-variate experiments.

So, this would be the component definitions for the most basic experiments:

<VariationExperiment experiment={ex} param="foo">
  <Variation name="bar">
    foo
  </Variation
  <Default>
    Defaulting
  </Default>
</VariationExperiment>

and this would be the component definition for a multi-variate experiment.

<VariationExperiment experiment={ex}>
  <Parameter param="foo">
    <Variation name="bar">
      fa
    </Variation>
  </Parameter>
  <Parameter param="bar">
    <Variation name="huzzah">
  </Parameter>
</VariationExperiment>

Note that implementation-wise ^ is basically the Namespace component. The only question that I couldn't seem to come to a conclusion to regards multi-variate experiments: would it ever make sense to marry components to combinations of parameters? Or should we always aim to have individual components deal with one parameter? For instance, consider the following basic 2x2 factorial experiment:

buttonColor = uniformChoice(choices=['blue', 'red'], unit=userid);
buttonText = uniformChoice(choices=['hi', 'bye'], unit=userid);

It makes sense to me to have a single Parameter component deal with setting the button blue / red and then another dealing with setting the text, but are there any cases you can think of where it would make sense for the two to be married?

4) I left some thoughts about logging in the PR you put up: https://github.com/HubSpot/react-experiments/pull/4. I think getting this exactly right while allowing consumers of the API to be super flexible in how they can define experiments in their app (but not to the point where users can implement bad experimental design practices...) is the most critical aspect of this library.

5) Also regarding 1), @mattrheault (another person at HubSpot who has put some work into this) has a PR that should make that way more efficient, thanks for pointing that out.

[rawls238]

closing as https://github.com/HubSpot/react-experiments/pull/6 should have covered these concerns. Thanks again @eytan!