Move dynamic bindings (resource-timing) to requests.js #18275
Conversation
| const analyticsVar = 'resourceTiming'; | ||
| dynamicBindings[binding] = | ||
| serializeResourceTiming(this.win, resourceTimingSpec); | ||
| expansionOptions.vars[analyticsVar] = binding; |
There was a problem hiding this comment.
the method has a side effect in this line which mutates the param.
removed and moved to the mapping to vendors.js.
| * We stop reporting after 1 minute. | ||
| * @private @const {number} | ||
| */ | ||
| this.maxResourceTimingReportingTime_ = Date.now() + (60 * 1000); |
There was a problem hiding this comment.
This doesn't work. It's each analytics instance has single reportingTime. the getMacroBindings only gets called before sending request so we can't start the timer with the getMacroBindings as well.
There was a problem hiding this comment.
good point. moved to requests.js to ensure start time is aligned with the start of amp-analytics element
| const analyticsVar = 'resourceTiming'; | ||
| dynamicBindings[binding] = | ||
| serializeResourceTiming(this.win, resourceTimingSpec); | ||
| expansionOptions.vars[analyticsVar] = binding; |
lannka
changed the title
lannka
force-pushed
the
move-dynamic-bindings
branch
from
85c61b3
to
a1dfafd
Compare
|
Comments addressed. PTAL |
| if (resourceTimingSpec) { | ||
| // Check if we're done reporting resource timing metrics before binding | ||
| // before binding the resource timing variable. | ||
| if (!resourceTimingSpec['done'] && |
There was a problem hiding this comment.
In the general I like the change and think it's OK. But we should be aware that this leads to a behavior change on using RESOURCE_TIMING. It resolves to RESOURCE_TIMING before, but resolves to an empty string now once done is set to true.
There was a problem hiding this comment.
That's right. it was too arbitrary that sometimes return empty string and sometimes it keeps the RESOURCE_TIMING, we'd better to unify the behavior.
| */ | ||
| export function getResourceTiming(win, spec, startTime) { | ||
| // Only allow collecting timing within 1s | ||
| if (spec && (Date.now() < (startTime + 60 * 1000))) { |
There was a problem hiding this comment.
nit: Is it better to save startTime + 60 * 1000 instead so that we don't need to calculate on every request? Or you think the save is too minor and we should focus on the naming instead.
There was a problem hiding this comment.
right, it does not make too much sense to store that time in requests.js
| [], newConfig(), 'https://ping.example.com/endpoint?rt='); | ||
| }); | ||
|
|
||
| it('should capture multiple matching resources', () => { |
There was a problem hiding this comment.
shouldn't the actual test be moved to test-resource-timing?
There was a problem hiding this comment.
there is already a test case in test-resource-timing. see L177
| expect(trigger['resourceTimingSpec']['responseAfter']).to.be.above(0); | ||
| }); | ||
|
|
||
| it('should url encode variables', () => { |
This is another preparation PR to unblock #6031