Add missing VAST 4.1 Macros #342
Conversation
ZacharieTFR
added
enhancement
feature
wip
| // <BlockedAdCategories> element is not parsed for now so the vastTracker | ||
| // can't replace the macro with element value automatically. | ||
| // The player need to pass it inside "macro" parameter when calling trackers | ||
| 'BLOCKEDADCATEGORIES', | ||
| 'ADCATEGORIES', // <Category> element is also not parsed for now. Same instructions as above | ||
| 'ADTYPE', //adType attribute of <Ad> element is not parsed for now. Same instructions as above | ||
| 'ADSERVINGID' // <AdServingId> element is also not parsed for now. Same instructions as above |
There was a problem hiding this comment.
I will open a PR when these elements are supported.
src/vast_tracker.js
Outdated
| */ | ||
| verificationNotExecuted(macros = {}) { | ||
| if ( | ||
| !(this.ad && this.ad.adVerifications && this.ad.adVerifications.length) |
There was a problem hiding this comment.
this is a bit hard to read, we could just write:
!this.ad || !this.ad.adVerifications || !this.ad.adVerifications.length
There was a problem hiding this comment.
but I'm not sure we should do this check. IMO this should break if you try to call it when there is no adVerifications. Or we should send an error. @rumesh what do you think?
There was a problem hiding this comment.
Y6es I agree, this function shouldn't be called if no adVerifications, but to be clean and robuste vast-client should send an error saying no adVerification.
Because if we do not have this check it will be an error like it wasn't handle properly by the vast-client.
So agree on this should break but not because accessing this.ad.adVerifications while it's not defined but because we throw an error in this case
…nt-js into missing-4.1-macros
…otExecuted tracker + unit test
src/util/util.js
Outdated
| const replacedUrlMacros = replaceUrlMacros(resolveURL, macros); | ||
| resolvedURLs.push(replacedUrlMacros); |
There was a problem hiding this comment.
Do we really need to store it in this tempory const ?
Why not just do this
resolvedURLs.push(
replaceUrlMacros(resolveURL, macros)
);
src/util/util.js
Outdated
| let supportedRemainingMacros = remainingMacros.filter( | ||
| macro => supportedMacros.indexOf(macro) > -1 | ||
| ); | ||
| if (!supportedRemainingMacros) { |
There was a problem hiding this comment.
| if (!supportedRemainingMacros) { | |
| if (supportedRemainingMacros.length === 0) { |
We know that filter return an array, and for execution performance it's better to do this otherwise browser have to "cast" the array as a boolean before executing this
src/util/util.js
Outdated
| const macro1 = `[${key}]`; | ||
| const macro2 = `%%${key}%%`; | ||
| replacedMacrosUrl = replacedMacrosUrl.replace(macro1, value); | ||
| replacedMacrosUrl = replacedMacrosUrl.replace(macro2, value); |
There was a problem hiding this comment.
Let use power of regex for better performance
// this will match [${key}] and %%${key}%%
let macroRegex = new RegExp(`(?:\[|%%)([${key}])(?:\]|%%)`, 'g');
replacedMacrosUrl = replacedMacrosUrl.replace(macroRegex, value);
There was a problem hiding this comment.
Not only better performance btw, with the regexp 'g' option it will replace all the occurence while a normal replace will only replace the first.
There was a problem hiding this comment.
Sure, i replaced it. It was a legacy code that i should have optimized before using 😅
Description
Add support for VAST 4.1 Macros. Macros can now be replaced by their values when trackers are called.
Issue
#336
TODO
Type