Separate scripts for different creative types

[musikele]

With this PR when puc is built (gulp build) output of that task will be multiple .js file. Each .js file is responsible for rendering ad of specific format:

• amp.js
• banner.js
• mobile.js
• native.js
• video.js

Before, when puc was built, in the output we would have:

• creative.js (for all formats except native)
• native-render.js (only for native format)

Not a single function was changed, all functions were just moved around (some to different files).
Also, some OOP concepts were removed to make treeshaking easier.

[dgirardi]

same for these.

(edit: great feeback, huh? I messed up and clicked the wrong buttons).

This is referring to the series of functions buildBanner, buildVideo as well as buildBannerDev, buildVideoDev, etc; they look copy-and-pasty and could be refactored to something like function buildDev(mediaType) {return function() ...}

[musikele]

created two generic functions, buildDev and buildProd, and other functions will call these.

[dgirardi]

this will have the effect of removing dist from the npm package; since that's what's backing jsDelivr, it will prevent deployment altogether.

https://docs.npmjs.com/cli/v8/commands/npm-publish

If there is a .gitignore or .npmignore file, then ignored files in that and all child directories will be excluded from the package. If both files exist, then the .gitignore is ignored, and only the .npmignore is used.

according to that, I think the solution is to add an .npmignore that contains everything in here except dist.

[musikele]

added file .npmignore

[musikele]

Docs PR: prebid/prebid.github.io#3645

[dgirardi]

LGTM with the limited testing I was able to do: both dist/creative.js and dist/banner.js work as expected with simple banner ads. dist/video.js is an exact copy of banner.js. I am completely ignorant of the mobile / AMP flow.

[jsnellbaker]

@musikele When you get a chance, can you review/resolve the conflicts in this PR? Thanks.

[jsnellbaker]

Is this comment in the right prod build function? I might have missed it, but what hb_format does the uid.js file connect with? Was this supposed to go in one of the older native prod files?

[musikele]

removed the deprecation warning

[jsnellbaker]

When I was running a basic test where I had a GAM creative setup with the new basic template (from the updated README.md) and calling a native creative, the new logic for the native.js ran into an issue here b/c the adId is not part of the nativeTag object that gets passed into the creative's renderAd() function; the adId is still stuck inside the targetingMap param in its natural format. See below for a copy of the creative code in its initial state when it gets to the code at this point:

  var ucTagData = {};
  ucTagData.adServerDomain = "";
  ucTagData.pubUrl = "http://test.localhost:9999/integrationExamples/gpt/hello_world_puc_adformat_nonsf.html?pbjs_debug=true";
  ucTagData.targetingMap = {"hb_adid":["3b4e49b4f268f3"],"hb_adid_appnexus":["3b4e49b4f268f3"],"hb_bidder":["appnexus"],"hb_bidder_appnexus":["appnexus"],"hb_format":["native"],"hb_format_appnexus":["native"],"hb_pb":["5.00"],"hb_pb_appnexus":["5.00"],"hb_size":["0x0"],"hb_size_appnexus":["0x0"]};
  ucTagData.requestAllAssets = true;

  try {
    ucTag.renderAd(document, ucTagData);
  } catch (e) {
    console.log(e);
  }

Because it can't find the adId, the rest of the rendering process is aborted.

In the previous way with the native-render.js approach, the adId a direct part of the object that's passed to the render function, see below for reference:

let pbNativeTagData = {};
pbNativeTagData.pubUrl = "https://sneezy.aws.rubiconproject.com/rp-eng-soln/prebid/prebid_1001_native_case4.html?pbjs_debug=true";
pbNativeTagData.adId = "3b8572ff0e96ab";
pbNativeTagData.requestAllAssets = true;
window.pbNativeTag.renderNativeAd(pbNativeTagData);

[musikele]

Hi @jsnellbaker , the error is in the readme.md file. The right creative should be:

<script src = "https://cdn.jsdelivr.net/npm/prebid-universal-creative@latest/dist/%%PATTERN:hb_format%%.js"></script>
<script>
  var ucTagData = {};
  ucTagData.adServerDomain = "";
  ucTagData.pubUrl = "%%PATTERN:url%%";
  ucTagData.targetingMap = %%PATTERN:TARGETINGMAP%%;
  ucTagData.hbPb = "%%PATTERN:hb_pb%%";
  ucTagData.hbFormat = "%%PATTERN:hb_format%%";
  ucTagData.adId = "%%PATTERN:hb_adid%%";
  ucTagData.requestAllAssets = true;

  try {
    ucTag.renderAd(document, ucTagData);
  } catch (e) {
    console.log(e);
  }
</script>

The creative is correct in the updated docs, for example at page https://deploy-preview-3645--prebid-docs-preview.netlify.app/adops/step-by-step.html .

Going to update the readme file.

[jsnellbaker]

From some initial tests I was running with a new GAM creative using the above code template, I'm not sure if simply writing 'native' here is accurate. From what I can tell, it seems like not all native setups that we support will be handled with just the code above.

Specifically the scenario where the native template is set within the creative's adserver. In that case, the shell of the creative's HTML is included above the script tag that loads the PUC native script (whether it's the new native.js or the previous incarnations of native-render.js or native-trk.js).
https://docs.prebid.org/prebid/native-implementation.html#native-template-in-adserver

If that's indeed the case, then the text here should highlight the restrictions (or give a link to the prebid.org native docs that can better explain what scenarios have specific requirements).

CCing @bretg for his thoughts on here (as it potentially relates to docs).

[musikele]

I've fixed the documentation PR to specify the document object as a first argument of the renderAd function.

In general, I don't really like the first scenario of the native approach, where the template is specified in GAM; it looks to me as something that's being carried on for compatibility with the past. It's also the scenario that is worse to customize because it requires to access GAM at every change. Anyway, the PR is compatible with that scenario too; i just tested it. Thanks for catching the mismatch.

[robertrmartinez]

Hi when I pull down this and try npm ci I get the following error:

I think maybe we should commit the changes to package-lock.json as well?

[musikele]

@robertrmartinez Added package-lock.json to fix npm ci.

[jsnellbaker]

@musikele

Thank you for keeping this PR healthy and making the tweaks to the things I found. TMK with what tests I was able to run, it seems like old regression files and the new files appear to be working. With proper documentation for the different usages, this should hopefully help people transition.

[patmmccann]

@musikele looks like we're ready to merge once conflicts are resolved?

[musikele]

@patmmccann rebased

[GLStephen]

@patmmccann came by to check up on things after our discussion today, is the README referencing https://cdn.jsdelivr.net/npm/prebid-universal-creative@latest/dist/%%PATTERN:hb_format%%.js out of sync with actual jsdeliver deployment right now? https://cdn.jsdelivr.net/npm/prebid-universal-creative@latest/dist/banner.js is not available. Looks like there is just some flux in state of things, but wanted to bring it up if it mattered.