Add support for extensions parsing #5
Conversation
spec/adbreak.spec.js
Outdated
| const vmap = new VMAP(xml); | ||
| const vmapWithExtensions = new VMAP(xmlWithExtensions); |
There was a problem hiding this comment.
Let's move the init setup for vmapWithExtensions directly inside the describe('Ad break with extensions' block
| </vmap:TrackingEvents> | ||
| </vmap:AdBreak> | ||
|
|
||
| <vmap:AdBreak breakType="linear" breakId="myid" timeOffset="00:10:23.125"> |
There was a problem hiding this comment.
we can remove these other adbreaks from this test xml. In the end we just need an xml with an adbreak which contains extensions
src/parser_utils.js
Outdated
| const nodeAttr = nodeAttrs[nodeAttrKey]; | ||
|
|
||
| if (nodeAttr.nodeName && nodeAttr.nodeValue) { | ||
| if (!parsedNode.attributes) { |
There was a problem hiding this comment.
We can avoid this check everytime by initializing parsedNode like this:
const parsedNode = {
attributes: {}
};
src/parser_utils.js
Outdated
| .forEach(childNode => { | ||
| if (!parsedNode.children) { | ||
| parsedNode.children = {}; | ||
| } |
There was a problem hiding this comment.
Same as above, plus I think it would be more consistent if this method returns everytime an object with the same structure.
So we can define its general structure on top and later add attributes and children in case there are some.
src/adbreak.js
Outdated
| @@ -30,7 +31,9 @@ class VMAPAdBreak { | |||
| } | |||
| break; | |||
| case 'Extensions': | |||
| this.extensions = node.childNodes; | |||
| this.extensions = childrenByName(node, 'vmap:Extension').map( | |||
There was a problem hiding this comment.
I put this as a reminder: since other parts of the parser still use node.localName, which is a deprecated property of the Node we api, we should always use node.nodeName plus this new helper function when possible.
see https://developer.mozilla.org/en-US/docs/Web/API/Node/localName
And maybe we can "tune" the childrenByName utility name to accept a name without the vmap: prefix and automatically search for children with and without the prefix
| @@ -0,0 +1,78 @@ | |||
| /** | |||
There was a problem hiding this comment.
we should add unit tests for all this methods too :p
To be sure their behaviour is correct one by one
src/parser_utils.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Parses a node text (for legacy support). |
There was a problem hiding this comment.
which legacy support?
src/parser_utils.js
Outdated
| for (let nodeAttrKey in nodeAttrs) { | ||
| const nodeAttr = nodeAttrs[nodeAttrKey]; | ||
|
|
||
| if (nodeAttr.nodeName && nodeAttr.nodeValue) { |
There was a problem hiding this comment.
what if nodeAttr.nodeValue === 0? probably 0, false, ... are values we want to parse no?
|
Looks good. We just need to use the new format here too https://github.com/dailymotion/vmap-js/blob/master/src/vmap.js#L24 And to update the |
Improved the parsing of adbreak extensions contained in the VMAP response.
Since no guidelines are specified for the content of extensions, the XML is parsed recursively with all its children