videojs / videojs-contrib-ads

A Tool for Building Video.js Ad Plugins
http://videojs.github.io/videojs-contrib-ads/
Other
379 stars 257 forks source link

feat: refactor macro logic and add support for disableDefaultMacros and macroNameOverrides parameters #544

Closed alex-barstow closed 1 year ago

alex-barstow commented 1 year ago

Description

This PR makes the following changes:

The new options are passed as properties of the existing customMacros object:

const customMacros = {
  disableDefaultMacros: false,
  macroNameOverrides: {
    '{player.id}': '{{PLAYER_ID}}',
  },
  '{{yourCustomMacro}}': 'yourValue',
  '{{otherCustomMacro}}': 'otherValue'
};

const adTagUrlWithMacrosReplaced = player.ads.adMacroReplacement(adTagUrl, true, customMacros);

Notes

One possible objection to this approach is that it introduces "magic properties" to customMacros that behave differently than other properties. They are implemented in such a way that there should not be any adverse side-effects (they are deleted from the customMacros object before it is processed), and I'm personally fond of the simplicity of this approach. It doesn't add any new parameters to adMacroReplacement() or the need for additional method calls.

Ideally, we would do something like this, but unfortunately it isn't backward-compatible:

const macroCustomizationOptions = {
  disableDefaultMacros: false,
  macroNameOverrides: {
    '{player.id}': '{{PLAYER_ID}}',
  },
  customMacros: {
    '{{yourCustomMacro}}': 'yourValue',
    '{{otherCustomMacro}}': 'otherValue'
  }
};

const adTagUrlWithMacrosReplaced = player.ads.adMacroReplacement(adTagUrl, true, macroCustomizationOptions);

If there are very strong objections to my approach, here are two other possible approaches:

1. Allow the customMacros argument to be a function that returns an object:

player.ads.adMacroReplacement(adTagUrl, true, () => {
  return {
    disableDefaultMacros: false,
    macroNameOverrides: {
      '{player.id}': '{{PLAYER_ID}}',
    },
    customMacros: {
      '{{yourCustomMacro}}': 'yourValue',
      '{{otherCustomMacro}}': 'otherValue'
    }
  };
})

This lets us adopt the ideal data structure in a backward-compatible way, but it adds a bit more conceptual complexity. It's also still very much a workaround.

2. Add 2 new methods that can be called before adMacroReplacement():

player.ads.disableDefaultMacros(true);
player.ads.macroNameOverrides({
 '{player.id}': '{{PLAYER_ID}}'
});
player.ads.adMacroReplacement(adTagUrl, true, customMacros)`

This is fairly clean, but it does require two separate method calls.

alex-barstow commented 1 year ago

I'm going to merge then follow-up with a PR to address Pat's comments.