Rethink namespace / experiment name as part of assignment algorithm in JS port only

[mattrheault]

Methods in question:

• getDefaultNamespaceName in namespace.js
• getDefaultExperimentName in experiment.js

I don't think that we should be using the namespace / experiment class names as part of the core planout assignment algorithm in the javascript port (specifically for the non-core compatible bundle).

The reasoning for this is that most frontend apps will compile, transpile, & minify their es6, or coffeescript, or typescript, or whatever. A common part of this process is to munge variable names to save on filesize. This means that planout.js cannot rely on the experiment / namespace class name to be consistent between builds.

Pre compile, transpile, & variable munge:

class FooExperiment extends planout.Exeriment {}
class BarNamespace extends planout.Namespace {}

Post compile, transpile, & variable munge:

var a = function a(_pe) {...}
var b = function b(_pn) {...}

This process often renames variables based on declaration order. If we were to swap the ordering of FooExperiment and BarNamespace, then enrollment would shift since the munged variable names will change.

What I propose:

All experiments & namespaces should have a static default name if none is set. EX: "GenericExperiment".

But we should provide an optional flag to allow people to use the classname as the default name if they choose. This flag should be disabled by default.

With this change, I would also recommend a major version bump to avoid shifting enrollment for people relying on the parsed experiment / namespace classname.

---

@rawls238 @eytan
/cc @geoffdaigle

[jcwilson]

Very interesting. I agree with your analysis, but maybe not the conclusion - at least for the core-compatible implementation. Anyone who has used AngularJS has run into a similar problem with the way one would describe their injected dependencies.

Would it makes sense to update the tests to load up the minified code rather than the human-readable versions to help catch things like this?

(specifically for the non-core compatible bundle)

I think we need to extra careful here, too, though. The python implementation contains similar logic to derive the experiment name from the class. However, they don't have the risk of the class name changing out from underneath them at some point. I don't see a way around this other than requiring (for safety's sake) that one should always do one of the following when defining an experiment: 1) provide the experiment name as a string such that it won't be affected by minification or 2) provide a salt (since it precludes the name from being used in the assignment calculation). Maybe this requirement is only enforced in the core-compatible implementation?

[rawls238]

I remember thinking about this a while ago - one idea I had when I was originally implementing this was that it should simply be a pseudo-"required" method when extending the Experiment or Namespace class precisely because of the minification issue.

IIRC we shouldn't have the same default name across experiments and namespaces since this affects the salt used in randomization and could inadvertently lead to bizarre and hard to debug issues, which is precisely why we pull from the default class name.

In hindsight, I think we should simply make setExperimentName and setNamespaceName required methods when instantiating a new version of a namespace/experiment.

Thoughts on this?

[mattrheault]

@rawls238 I like the approach of making experiment / namespace names required. Good thinking 👍

I'll put together a PR for that, and then combine those changes with the changes in #54 to prep for a v4.0 release.

[geoffdaigle]

@mattrheault Might be helpful to toss in a "why do I need to set the name?" section in the docs in case devs care (or in case we forget).

[mattrheault]

Addressed in - #59
V4.0 release - #58