Add VAST 4.0 support #52
Conversation
There was a problem hiding this comment.
No major changes, I think.
Great that we're adding so many tests as well. Coverage seems good if I run it locally. Is there any major feature we're not covering yet?
To be consistent with iab-vast-model, let's also look into bumping the dependency versions and the minimum version of Node.
Additionally, the LICENSE year is probably outdated here too.
| @@ -5,11 +5,17 @@ export default ($ad, options) => { | |||
| const wrapper = new Wrapper() | |||
| const $wrapper = $ad.wrapper | |||
| inheritAd($ad, $wrapper, wrapper, options) | |||
| if ($wrapper.followAdditionalWrappers != null) { | |||
There was a problem hiding this comment.
followAdditionalWrappers has a default value of true, and allowMultipleAds has a default value of false in the VAST 4.0 spec. So we cannot simply do the assignment.
src/factory/extension.js
Outdated
| @@ -1,8 +1,9 @@ | |||
| import {Extension} from 'iab-vast-model' | |||
|
|
|||
| export default ($extension) => { | |||
| // console.log($extension) | |||
src/factory/creative.js
Outdated
| if ($creative.linear) { | ||
| return createLinear($creative, options) | ||
| creative.linear = createLinear($creative, options) |
There was a problem hiding this comment.
Might still change depending on iab-vast-model PR.
I also seem to remember that one of the reasons why we had to change this was because it's not an else if, since they can coexist?
There was a problem hiding this comment.
Yes, a or a . And an optional .
There was a problem hiding this comment.
Let's try again...
Yes, it either a Linear or a NonLinearAds. And an optional CompanionAds.
src/factory/universal-ad-id.js
Outdated
| @@ -0,0 +1,9 @@ | |||
| export default (universalAdId, $universalAdId) => { | |||
| if ($universalAdId.idRegistry) { | |||
There was a problem hiding this comment.
We seem to be mixing if (object) and if (object != null). It's not a huge issue so I don't want it to block this PR, but let's try to make sure that everything inside if (...) evaluates to a boolean in the future.
src/factory/viewable-impression.js
Outdated
| const viewableImpression = new ViewableImpression() | ||
| viewableImpression.id = $viewableImpression.id | ||
| if ($viewableImpression.viewable != null) { | ||
| viewableImpression.viewables.push(...$viewableImpression.viewable.map(i => i._value)) |
There was a problem hiding this comment.
I don't like i as a variable name if it's not a counter.
| @@ -2,7 +2,7 @@ import TYPES from './types' | |||
| import parseTime from '../util/parse-time' | |||
|
|
|||
| export default { | |||
| [TYPES.bool]: (str) => (str === 'true'), | |||
| [TYPES.bool]: (str) => (str === 'true' || str === '1'), | |||
There was a problem hiding this comment.
No, it is from the IAB XML test-files. They sometimes use "1" rather than "true".
CircleCi will fail until the model has been updated.