Refactored randomization module #24 #30
Conversation
|
|
||
| // Try to load the selected condition from the cache | ||
| if(conditionSet.name in conditionCache) | ||
| return conditionCache[conditionSet.name]; |
There was a problem hiding this comment.
I think always using braces is a better practice (someone always comes along and adds a condition without realizing) but let's do that with eslint and eslint --fix everything vs. bothering to do it here. I'll file an issue
There was a problem hiding this comment.
(I mean it's not that terrible here because it's return or throwing below, but if someone comes along and inserts a new line before the return things fail in new and interesting ways)
| throw "Conditions must have unique names." | ||
| conditionNames.add(condition.name); | ||
| } | ||
| let randomValue = Math.random(); |
There was a problem hiding this comment.
I went down the rabbit-hole of how Math.random works nowadays, I guess it's using https://en.wikipedia.org/wiki/Xoroshiro128%2B now, which sounds better than I remember!
There was a problem hiding this comment.
I thought about using the Web Crypto API for better randomness. I think we're OK with Math.random in this context, since the goal is just to divide users into a set of cohorts with target relative populations. Those cohorts will change (e.g., users withdrawing), and if researchers run statistical tests they'll have to account for cohort size.
| @@ -53,3 +53,6 @@ export { linkExposure } | |||
|
|
|||
| import * as socialMediaLinkSharing from "./socialMediaLinkSharing.js" | |||
| export { socialMediaLinkSharing } | |||
|
|
|||
| import * as randomization from "./randomization.js" | |||
| export { randomization } | |||
There was a problem hiding this comment.
I wonder if we could do these as one line like: export { randomization } from './randomization.js? I haven't tried it yet but remember seeing it on MDN.
There was a problem hiding this comment.
I looked into this. There's a pending proposal for namespace re-exports: https://github.com/tc39/proposal-export-ns-from
There was a problem hiding this comment.
Hm, wasn't that merged in tc39/ecma262#1174 ? Although looking at the compatibility table on MDN we should probably avoid it for now anyway.
There was a problem hiding this comment.
Ah, I misunderstood the status of the proposal. Since we already need to use recent browser versions because of various WebExtensions APIs, I think we could switch to this syntax.
There was a problem hiding this comment.
I'll leave it to you whether JS Math.random is high-quality enough for the needs here, my knowledge about Math.random was a little behind the times so that was interesting to learn about anyway.
No description provided.