Exposes experiment variants as variable for URL replacement. #3868
Conversation
|
Over to @erwinmombay. I'll take a look later. |
| user.assert(variants, | ||
| 'To use variable VARIANT, amp-experiment should be configured'); | ||
| user.assert(variants[experiment] !== undefined, | ||
| `The value passed to VARIANT() is not a valid experiment name: |
There was a problem hiding this comment.
is new line intentional here? this will add a \n
There was a problem hiding this comment.
Thought IDE would be smart here :-). Fixed.
| if (variantName) { | ||
| results[experimentName] = variantName; | ||
| } | ||
| results[experimentName] = variantName; |
There was a problem hiding this comment.
think we can return results here then we can get rid of .then(() => results)
cc @jridgewell
There was a problem hiding this comment.
Nope, we're inside a Promise.all here, so we'd get an array of [results, results, results, ...].
There was a problem hiding this comment.
ah gotcha, didnt see we were inside a .map. thanks
There was a problem hiding this comment.
Very legit questioning. It's indeed a bit overwhelming here. Let me break some inlining to gain some readability :-)
| results[experimentName] = variantName; | ||
| }); | ||
| }); | ||
| this.experimentVariants = Promise.all(variants).then(() => results); |
There was a problem hiding this comment.
much more readable. much appreciated
There was a problem hiding this comment.
is there a type declaration for experimentVariants ? and does it need to be public?
|
LGTM, deferring to @cramforce for final LGTM |
| // Returns assigned variant name for the given experiment. | ||
| this.set_('VARIANT', experiment => { | ||
| return this.variants_.then(variants => { | ||
| user.assert(variants, |
There was a problem hiding this comment.
For better or worse, we only report errors for substitutions. This allows e.g. analytics vendors to have generic configuration that works with and without variants.
Return null if none is defined.
There was a problem hiding this comment.
Do you mean:
if (error) {
user.error(...);
return null;
}
If returning null will end up with an empty string, it's identical to user.assert. Because in the replace logic, we catch all errors. See one of my test cases for example.
There was a problem hiding this comment.
Yes, you are correct. Disregard.
|
LGTM |
For #1411