'isomorphic' in description is misleading

[giogonzo]

There's maybe 2-3 things that an 'isomorphic' react lib should do to differentiate from a 'non isomorphic' one, e.g.

• avoid basing functionality on non-universal lifecycle hooks (i.e. componentDidMount)
• avoid unguarded window and document accesses

This lib is not doing any of the above, and thus I think the term 'isomorphic' in description is misleading

I've seen this: #1

and this part of readme:

get, set and del Get, set and delete stored experiments. By default these functions use browser cookies. When rendering server side these should be changed appropriately.

and in my opinion they're not enough. You should either provide a clear API for server side usage, or drop 'isomorphic' from description

I see the accessors are already configurable via props: https://github.com/olahol/react-ab/blob/master/react-ab.js#L78-L85

One simple addition that would make this easier for users could be: either accept those props from context too, or provide a wrapper component that renders the Experiment component with appropriate accessors props, obtained from context Server side users would just need to provide accessors in context (e.g. simple functions accessing node req.cookies and res) in their top-level render

As a sidenote, also componentWillMount setting state (https://github.com/olahol/react-ab/blob/master/react-ab.js#L99-L111) looks not super safe for universal rendering

[olahol]

Here is an example of how I use the component server side:

import express from "express";
import cookieParser from "cookie-parser";

import React from "react/addons";
import { Experiment, Variant } from "react-ab";

var App = React.createClass({
  choice: function (experiment, variant, index) {
    console.log(experiment, variant, index);
  }

  , render: function () {
    return (
      <div>
        <Experiment {...this.props} onChoice={this.choice} name="tagline">
          <Variant name="normal">
            <h1> A A/B testing component for React </h1>
          </Variant>
          <Variant name="enterprise">
            <h1> A vertically integrated React component </h1>
          </Variant>
          <Variant name="lies">
            <h1> One weird React component that will increase your metrics by 100%! </h1>
          </Variant>
        </Experiment>
      </div>
    );
  }
});

var app = express();

app.use(cookieParser());

app.get("/", function (req, res) {
  res.send(React.renderToString(<App
    get={(x) => req.cookies[x]}
    set={(x, y) => res.cookie(x, y)}
    delete={res.clearCookie}
  />));
});

app.listen(3000);

I don't see how it could be any simpler.

• I assume that the component will most often be used only on the client side which is why I bundle and make default the browser cookie methods for persisting experiments.
• componentWillMount is universal as per React documentation https://facebook.github.io/react/docs/component-specs.html#mounting-componentwillmount

I also don't see a problem in setting state in componentWillMount seeing as I assume that all of the functions get, set, delete are synchronous which will make state available before the first call to render.

I hesitate to add the context overrides since context does not have documentation yet.

[giogonzo]

Thanks for the example! I suggest you to add it to the readme too? (even a simpler one just showing get, set and delete override would work)

Yes, it is indeed simple if your Experiment is one level deep in your render tree - advantages of the context api would be with "deep" Experiments. I'm not saying to have a context-only api, but a little opt-in wrapper component would be great (same as what, for instance, alt does with AltContainer)

I hesitate to add the context overrides since context does not have documentation yet.

Right, and it is still rapidly evolving so this makes sense in general. But as long as you have props as main api this should not be an issue

[olahol]

Yes I will add it to the readme, and also do something with context so that passing these functions becomes easier, thank you for your input.

[giogonzo]

Sounds great. Thanks for caring! On Aug 24, 2015 8:04 PM, "Ola Holmström" notifications@github.com wrote:

Yes I will add it to the readme, and also do something context so that passing these functions becomes easier, thank you for your input.

— Reply to this email directly or view it on GitHub https://github.com/olahol/react-ab/issues/2#issuecomment-134320085.

[olahol]

I've added more documentation about how to achieve universality, I've also added a way to pass get, set , clear via context by setting getExperiment, setExperiment, clearExperiment on childContext. Check it out to see if it works for you.

[giogonzo]

Hi, this looks good! Didn't have a chance to actually test it though On Aug 24, 2015 9:49 PM, "Ola Holmström" notifications@github.com wrote:

I've added more documentation about how to achieve universality, I've also added a way to pass get, set , clear via context by setting getExperiment, setExperiment, clearExperiment on childContext. Check it out to see if it works for you.

— Reply to this email directly or view it on GitHub https://github.com/olahol/react-ab/issues/2#issuecomment-134354935.