Investigate ways to avoid assign.toString() to grab param names

[mattrheault]

https://github.com/HubSpot/PlanOut.js/blob/master/es6/experiment.js#L32

Ideally we should have a way to proxy params when they are set so that the names exist on the experiment class instance.

[rawls238]

We do have a way to do this though through getParamNames().

The whole point of this magic is that users don't have to explicitly worry about this every time when setting up an experiment. In my mind there are two possible other ways to do this:

1. Have getParamNames() be forced to be manually set. My view is that this is basically having two sources of truth and makes things prone to careless bugs.

2. Change the assign method to be more explicit about the parameters being set (perhaps through setting them via a JSON object or something), but I think this would just make the API more cumbersome and less flexible (admittedly being rather vague here about this).

While I agree that having this sort of magic isn't the best thing from a code readability POV, I think it's worth it for the benefits that it brings unless there's evidence that it doesn't work well.

Let me know what you think

[mattrheault]

@rawls238 for context, we got bit by this a few weeks ago.

What we did:

// makeExperiment factory
const makeExperiment = ({
  name = 'GenericExperiment',
  setupParams = null,
  ...
} = {}) => {
  ...

  const ComposedExperiment = class HublabsExperiment extends planout.Experiment {
    ...

    assign(params, args = {}) {
      ...
      const paramSetter = params.set.bind(params);
      setupParams(paramSetter, {choose, flipCoin}, args);
      ...
    }

    ...
  }

  ...

  return new ComposedExperiment(...);
}

// Example usage
makeExperiment({
  setupParams: (setParam, {flipCoin}) => {
    setParam('which_group', flipCoin());
  }
});

Because we proxied the actual assignment of parameter names to another method (setupParams), the toString() of assign would never contain any .set() calls to match.

I don't think that this issue is super high priority since I doubt that many people will ever run into this problem. We only discovered this because we have a bunch of internal wrappers around planout.js classes to make building experiments a bit less verbose, and to abstract away classical inheritance as an implementation detail of planout.

All that aside, I think this is an example of planout.js doing something that I wouldn't quite expect it to do. I think that there's some potential in the 2nd solution you proposed, but I agree in that I'd like to find a solution that doesn't make experiment classes feel gross to work with.

[rawls238]

I see, but having something like the second solution would constrain the flexibility of assignment (at least with what I have in mind). I have some vague idea on how to make it better, I'll tag you in the PR if I try it and works and makes sense