Extended experiments URL parser to handle originalHash #4536
Conversation
| let queryString = win.location.search; | ||
| // If we're inside the AMP viewer, the hash fragment will have been moved | ||
| // to the originalHash field. | ||
| const hash = win.location.originalHash || win.location.hash; |
There was a problem hiding this comment.
We really need a hash service.
There was a problem hiding this comment.
I don't disagree. Is this something that's near enough term that I should block this on? (Would rather not, since this PR is required for the full A4A experiment to get running.)
There was a problem hiding this comment.
Nah, let's go ahead with this for now.
There was a problem hiding this comment.
viewer.getFragment() is the preferred way to get hash.
There was a problem hiding this comment.
Woo. Is it really necessary to go to a Promise just to get at the hash? This will be running in the code that decides whether to execute A4A or the existing 3p iframe amp-ad implementation. I'd prefer not to have to block for an unknown time before making this decision. Is there a (small, fast) synchronous way to do this?
Also, it looks like viewer.getFragment() only accesses location.hash, but when I look at AMP docs in the viewer through Chrome dev console, I only see stuff populated in location.originalHash. Looks like viewer.constructor() moves it from hash to originalHash? So will getFragment() even get at that?
But, really, what I want here is "the URL parameter exp, whether it's set in the host page URL search fragment or the hash/originalHash fragment". It looks like viewer.getParam() might give me that? Would that be a simpler way to go here?
There was a problem hiding this comment.
Yes. If exp is the only thing you want, viewer.getParam('exp') is a better choice. Right now originalHash is only used to defer mode. Viewer param can be passed to runtime in multiple places, see this code.
Please see previous discussion
|
Migrated to using |
| it(`should find viewer param when pattern is ${hashBase}`, () => { | ||
| win.location.hash = hashBase.replace('PARAM', 'a4a:-1'); | ||
| installViewerService(win); | ||
| viewerFor(win); |
There was a problem hiding this comment.
is this call necessary? It seems to be a simple getter with no side-effect.
There was a problem hiding this comment.
Good catch! I admit that this sequence is a mystery to me. :-( This goes back to "couldn't figure out how to get Viewer to (re-)parse the hash parameters." This sequence was the result of mutational testing, and it was the one that worked. I stopped when it ran and passed tests, and didn't try all the ways I could strip it down. :-)
I dropped the viewerFor line.
There was a problem hiding this comment.
I think it's a problem that we didn't make it consistent.
for many other services, calling xxxFor(win) will install the service.
| expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES, | ||
| INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true; | ||
| expect(win.document.cookie).to.be.null; | ||
| expect(rand.called, 'rand called at least once').to.be.false; |
There was a problem hiding this comment.
try this: expect(rand).to.not.be.called
There was a problem hiding this comment.
Oh, nifty! I didn't realize you could do that. Changed in multiple places. Thanks!
|
LGTM |
| expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES, | ||
| INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true; | ||
| expect(win.document.cookie).to.be.null; | ||
| expect(rand.called, 'rand called at least once').to.be.false; |
There was a problem hiding this comment.
one more left.
I think you can also remove the message, as it will say something similar for you.
There was a problem hiding this comment.
D'oh! Ok, this time I did a global search for rand.called and for to.not.be.called. I think (hope?) I got them all. Also removed messages. PTAL.
* Extended experiments URL parser to handle originalHash * Intermediate state: Update in place, but working on tests. Do not merge to master. * Update method of hash parsing for URL params. * Remove a debugging-only log message. * Lint fixes. * Removed unnecessary viewerFor call. * Updated expectations for rand.called. * Updated expectations for rand.called.
* Extended experiments URL parser to handle originalHash * Intermediate state: Update in place, but working on tests. Do not merge to master. * Update method of hash parsing for URL params. * Remove a debugging-only log message. * Lint fixes. * Removed unnecessary viewerFor call. * Updated expectations for rand.called. * Updated expectations for rand.called.
When the target page is served via the Google Search page, experiment parameters will be passed in the hash fragment. In turn, that is translated to
win.location.originalHashby the AMP viewer. This change allows the A4A experiments configuration URL parser to find experiment control parameters in theoriginalHashorhashfields.