Fix allowMultipleAds option when resolving wrapper #375
Conversation
…when resolving wrapper
src/parser/vast_parser.js
Outdated
| let allowMultipleAds; | ||
| if (this.parsingOptions.allowMultipleAds) { | ||
| allowMultipleAds = this.parsingOptions.allowMultipleAds; | ||
| } else { | ||
| allowMultipleAds = ad.allowMultipleAds; | ||
| } |
There was a problem hiding this comment.
| let allowMultipleAds; | |
| if (this.parsingOptions.allowMultipleAds) { | |
| allowMultipleAds = this.parsingOptions.allowMultipleAds; | |
| } else { | |
| allowMultipleAds = ad.allowMultipleAds; | |
| } | |
| const allowMultipleAds = this.parsingOptions.allowMultipleAds || ad.allowMultipleAds |
There was a problem hiding this comment.
Much cleaner ! ✨
There was a problem hiding this comment.
Following my last comment we might want to use ?? (Nullish coalescing) operator
| @@ -117,6 +117,8 @@ By default the fully parsed `VASTResponse` contains all the Ads contained in the | |||
| - `urlHandler: URLHandler` - Custom urlhandler to be used instead of the default ones [`urlhandlers`](../../src/urlhandlers) | |||
| - `urlhandler: URLHandler` - Fulfills the same purpose as `urlHandler`, which is the preferred parameter to use | |||
| - `resolveAll: Boolean` - Allows you to parse all the ads contained in the VAST or to parse them ad by ad or adPod by adPod (default `true`) | |||
| - `allowMultipleAds: Boolean` - A Boolean value that identifies whether multiple ads are allowed in the requested VAST response. This will override any value of allowMultipleAds attribute set in the VAST | |||
There was a problem hiding this comment.
Maybe the doc is misleading, it says it will override any value of allowMultipleAds attribute set in the VAST but actually if I pass allowMultipleAds: false to it and the VAST has allowMultipleAds set to true, we will take the one from the VAST
There was a problem hiding this comment.
Actually, we do want to replace any vast value if allowMultipleAds was provided in the option. Considering the null coalescing operator it will do so 👍
|
LGTM 🥇 |
| withCredentials: options.withCredentials, | ||
| }; | ||
|
|
||
| this.maxWrapperDepth = options.wrapperLimit || DEFAULT_MAX_WRAPPER_DEPTH; | ||
| this.parentURLs = []; | ||
| this.parsingOptions = { allowMultipleAds: options.allowMultipleAds }; | ||
| this.remainingAds = []; | ||
| this.rootErrorURLTemplates = []; | ||
| this.rootURL = ''; |
There was a problem hiding this comment.
Any reason why this re order is need? or was it generate by prettier 2.0?
There was a problem hiding this comment.
There's no real need, I just thought it would be more readable 🤷♂️
Description
The issue was raised through #371, we can already override the vast value of allowMultipleAds by setting its value inside the option parameter of following public methods:
get(url, options)API linkgetAndParseVAST(url, options)API linkparseVAST(vastXml, options)API linkBUT
In the context of a wrapper, when resolving, the allowMultipleAds value defined inside the unwrapped vast was not replaced by the value given in the option. This PR fix it.
Issue
#371
Type