Remove CONTENT- and MEDIAPLAYHEAD macro and fix timecode generation #358
Conversation
There was a problem hiding this comment.
Thanks for contributing @klipstein ! Actually you're right CONTENTPLAYHEAD and MEDIAPLAYHEAD can only be set by the player as we do not have the value for it in the tracker. Good catch for the double encoding 👍. Let's wait for another approval
src/util/util.js
Outdated
| function leftpad(str, len) { | ||
| str = String(str); | ||
| if (str.length < len) { | ||
| return ( | ||
| range(0, 8 - str.length, false) | ||
| range(0, len - str.length, false) |
There was a problem hiding this comment.
First of all thank you for this PR, and for the contribution.
WDYT to put the second parameter as optional ? with value to 8, as before ?
Also I think we shouldn't reaffect str
I would like to propose this, if you are ok:
function leftpad(input, len = 8) { const str = String(input);
Then the previous call to leftpad stay the same (and if anyone was using this outside the library, it will not break the code)
There was a problem hiding this comment.
I thought that those utils are an implementation detail of vast-client and are not exposed. Your proposal makes sense and I'll adjust the pull as suggested.
src/vast_tracker.js
Outdated
| @@ -532,16 +531,16 @@ export class VASTTracker extends EventEmitter { | |||
| // Use the provided fallbackClickThroughURL as a fallback | |||
| const clickThroughURLTemplate = | |||
| this.clickThroughURLTemplate || fallbackClickThroughURL; | |||
| // clone second usage of macros, which get mutated inside resolveURLTemplates | |||
| const clonedMacros = JSON.parse(JSON.stringify(macros)); | |||
There was a problem hiding this comment.
As macros is 1 level plain object, what about using destruct to clone the object ?
| const clonedMacros = JSON.parse(JSON.stringify(macros)); | |
| const clonedMacros = {...macros}; |
Performance wise it's better I think.
There was a problem hiding this comment.
I wanted to use {...macros} but eslint complained with Unexpected token ... and I did not wanted to fiddle with the eslint config. I'll give it a try.
There was a problem hiding this comment.
I've upgraded eslint dependencies and used babel-eslint as parser so that the spread syntax is now supported. Also had to disable the rule no-prototype-builtins because some parts of the code conflict with this newly enabled rule (see https://eslint.org/docs/rules/no-prototype-builtins) and I didn't want to touch that.
Eslint complained about `Unexpected token ...` so I've upgraded eslint and switched to babel-eslint parser
Description
While playing around with this library, I tested it by accident with a VAST that contained the macro
[CONTENTPLAYHEAD]and I've wondered why it was filled with such a value:0.0011111111111111111%3A0.06666666666666667%3A4.65.I also wondered why the
CONTENTPLAYHEADwas continuously updated whereas it should be set to the content playhead where the ad started. Same also for theMEDIAPLAYHEADand I think those two macros can only be set from the outside to the VAST Tracker. Another finding was, that themacroswhich get passed into theclickmethod get mutated in two places, once for the click-tracking and once for calculating the click-through URL. This can lead to double-encoded macro values.This pull request tries to fix the mentioned issues.
Type