VAST 3.0 Companion ads

[kobawan]

Description

Support VAST 3.0 Companion ads

Added properties:

CreativeCompanion:

• required: String|null

CompanionAd:

• assetWidth: Number|null
• assetHeight: Number|null
• expandedWidth: Number|null
• expandedHeight: Number|null
• apiFramework: String|null
• adSlotID: String|null
• adParameters: String|null
• xmlEncoded: Boolean|null

Renamed properties:

CompanionAd:

• staticResources: Array<{ url: String, creativeType: String|null }> (Renamed from staticResource)
• htmlResources: Array (Renamed from htmlResource)
• iframeResources: Array (Renamed from iframeResource)

Removed property:

CreativeCompanion:

• trackingEvents: Object
• type: String|null (It is now part of staticResources)

Type

• Breaking change
• Enhancement
• Fix
• Documentation
• Tooling

[eliamaino-fp]

why are we using forEach here and map on line 21?

[kobawan]

because companionAd.trackingEvents is an object not an array

[eliamaino-fp]

Not sure I get what you mean, both times you chain to childrenByName which always returns an array.

[eliamaino-fp]

Maybe we can make the scenario in which we call childrenByName + some array iteration faster, by making children extraction accepting a custom function to apply:

function applyToChildrenByName(node, nodeName, fn) {
  const children = [];
  const childNodes = node.childNodes;

  for (const childKey in childNodes) {
    const child = childNodes[childKey];

    if (child.nodeName === name) {
      children.push(fn(child)); // This will save us one iteration
    }
  }
  return children;
}

Or

function childrenByName(node, nodeName, fn) {
  const hasFunctionToApply = typeof fn === 'function'
  const children = [];
  const childNodes = node.childNodes;

  for (const childKey in childNodes) {
    const child = childNodes[childKey];

    if (child.nodeName === name) {
      hasFunctionToApply ? children.push(fn(child)) : children.push(child);
    }
  }
  return children;
}

What do you think about this idea?

[kobawan]

I think this will make the code less readable in my opinion. Mostly because we would be passing the entire parsing function as a callback :/ I also don't think that iterating twice in this case is a problem (if it would be a large array, maybe...). I don't have a very strong opinion on this though.

[eliamaino-fp]

You are right, it doesn't have a noticeable impact on execution perf.

[eliamaino-fp]

You are right, it doesn't have a noticeable impact on execution perf.

[eliamaino-fp]

I like this approach of passing at least the attributes to the constructor (and the class logic will take care of assigning them to the right properties).

Maybe we can factor this in an util method in order to use it in the future too, something like getAttributes:

function getAttributes(node) {
  const attributes = {};
  const nodeAttributes = node.attributes;

  for (let i = 0; i < companionAttrs.length; i++) {
    attributes[nodeAttributes[i].nodeName] = nodeAttributes[i].nodeValue;
  }

  return attributes
}

[kobawan]

created function parseAttributes

[eliamaino-fp]

According to the VAST spec

Each element must specify at least one resource file that may be one of:
<StaticResource>, <IFrameResource> or <HTMLResource>

What is the approach we want to take when multiple resources are provided?

[kobawan]

I didn't understand from the specs whether you could have multiple StaticResources for example, or just multiple resources of different types. either way we were still only assigning one resource per type previously

[kobawan]

added support for multiple of the same resources

[eliamaino-fp]

Looks good