Generalized client side experiments#7870
Conversation
| */ | ||
| function processQueryParams(queryString) { | ||
| Object.keys(experimentList).forEach(function (key) { | ||
| var regex = new RegExp(key + '=(true|false)'); |
There was a problem hiding this comment.
Though not strictly necessary, it might be nice to make experiment names (and true/false values) case-insensitive. Also to accept 0 or 1 as values.
There was a problem hiding this comment.
Note: This regex is ambiguous in the case that one queryparam forms the end of another. In the case of your topInstructions experiment, if somebody ever came along and added an Instructions experiment we would also update Instructions every time you tried to set topInstructions via queryparam.
Small and unlikely, but hopefully an easy fix - put a word boundary \b at the beginning of your regex.
|
Approach seems sound and well-tested. Comments are speculative improvements, but take them or leave them. LGTM. |
| experiments.isEnabled = function (key) { | ||
| if (!performedLoad) { | ||
| loadFromLocalStorage(); | ||
| processQueryParams(window.location.search); |
There was a problem hiding this comment.
I'm not sure the one-time load you're doing here gains you anything. In theory, it'd be equally valid to check localstorage and the querystring on demand for each isEnabled check, and it's possible that it's a hair faster as the list of experiments grows to do so - if we have ten experiments and only two are used on a page, you only create/run two RegExp objects against the querystring, instead of ten.
There was a problem hiding this comment.
I think you're right. I don't think we'll ever really need to be in a place where we need to ask the question "What are all the enabled experiments", so tracking them all doesn't have a lot of value. Really, the only part we want is "Is this experiment enabled".
|
Refactored this significantly. This feels a lot better and simpler |
| */ | ||
| experiments.isEnabled = function (key) { | ||
| var enabled; | ||
| var queryString = this.__TestInterface__.queryString || |
There was a problem hiding this comment.
I don't love this. Can you think of a better way of doing this?
There was a problem hiding this comment.
At minimum, extracting a getQueryString helper could hide the kludge.
Then, you might be able to have the test monkeypatch over the getQueryString method, so you don't need to expose any special test interface here. I'm not sure if that's 'cleaner' than what you already have though.
It's too bad you can't just overwrite window.location.search since you already have to run this test in a browser context. You could always set up this method so that you have to inject window into it, but that's a step away from the simplicity of your interface.
827a0e4 to
a65a0a9
Compare
Provide a generalized way of experimenting on the client (currently assumes experiments are toggleable).
It's possible I tried to generalize this too early, as it's only needed for one other thing (topInstructions, and soon runModeIndicators). I'm also not too sure I love the design. There's some weirdness in that if we never check to see if any experiments are enabled, we won't parse your query params. Or maybe that's not that weird. I guess we could do the parsing as part of init if we were concerned.
I'm interested in hearing what you think of this.