Analytics: Enable parentPostMessage
#17641
Conversation
zhouyx
force-pushed
the
analytics-postmessage
branch
from
3c17378
to
7808661
Compare
| if (!trigger['on'] || !trigger['request']) { | ||
| const hasRequestOrPostMessage = trigger['request'] || | ||
| (trigger['parentPostMessage'] && this.isInabox_); | ||
| if (!trigger['on'] || !hasRequestOrPostMessage) { | ||
| this.user().error(TAG, '"on" and "request" ' + | ||
| 'attributes are required for data to be collected.'); |
There was a problem hiding this comment.
This is now a somewhat misleading error message, since request is no longer required. For example, if someone misspelled parentPostMessage they might think this was telling them they also needed to add request
There was a problem hiding this comment.
That's a good point. I refined the error message a bit in inabox and non inabox case.
|
|
||
| if (!request) { | ||
| if (requestName != undefined && !request) { | ||
| const TAG = this.getName_(); | ||
| this.user().error(TAG, 'Ignoring event. Request string ' + | ||
| 'not found: ', trigger['request']); |
There was a problem hiding this comment.
Weird to say we're ignoring the event if we might not be?
There was a problem hiding this comment.
Rephrase to ignoring request for event. Request ..... Let me know if that's better.
| expect(postMessageSpy).to.have.been.called; | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Could we have a test for when there's both a post message and a request?
There was a problem hiding this comment.
added test
inabox case: w/ request and postMessage, w/o request postMessage, only postMessage
non inabox case: with postMessage and request
| * return strings, promises, etc. | ||
| * @return {Promise<string>} | ||
| */ | ||
| export function expandPostMessage(baseInstance, msg, |
There was a problem hiding this comment.
are we able to just reuse the expandTemplate method?
There was a problem hiding this comment.
Sure we can also choose to expose the expandTemplate getExtraUrlParams getExtraUrlParamsString and constructExtraUrlParamStrs to amp-analytics.js
But I feel it's cleaner this way. Let me know if you have strong opinion.
zhouyx
force-pushed
the
analytics-postmessage
branch
2 times, most recently
from
0a3e0c9
to
3a05e06
Compare
|
One thing just crossed my mind. Will the new error message on the same error break publishers error analysis? |
zhouyx
force-pushed
the
analytics-postmessage
branch
from
3a05e06
to
8b07771
Compare
* init commit * fix linter * refine error message * code style
Did this ever end up happening? Do you need me to do it? |
I'm sorry but I haven't documented the feature yet. It would be great if you can help! Our analytics documentation is a little bit outdated not only for this feature. @jeffjose let's plan a mini sprint to update our docs after AMP conf. WDYT |
As we agreed, add the
parentPostMessageto analyticstriggerconfig, and post the message to parent if is inabox runtime.Note: I decided that automatically appending
${extraUrlParams}doesn't make much sense in this case. However one can still includeextraUrlParamswithparentPostMessage: foo ${extraUrlParams} barI'll submit doc change in another PR.