Prebid Core: Define "VIDEO" compile time feature flag #9543
Conversation
There was a problem hiding this comment.
Thank you for putting this together! I think there's a few more things that can be tagged out. My strategy would be to build-bundle-verbose --disable VIDEO to get a somewhat legible compilation output and try to spot the stragglers. here's what that produces with this version, and a few of the targets that I see:
- custom renderers have some video-specific logic. There's a good chance that if you don't need video you also don't need custom renderers at all, but I'm not sure that's always the case. It may be worth tagging renderers as its own feature (maybe separately from this PR).
- there's some arcane adpod-related logic which should not be in core in the first place, and I'm almost sure you can do without if you're not interested in video.
- because the adpod module
imports some video symbols, it prevents them from being removed as dead code. this should be fixable replacing the importedcallPrebidCachewithgetHook('callPrebidCache'). - some special case for video in renderAd
You may also want to tag out ORTB conversion utitlies for video (here's the native tagging for reference). This does not save any bytes in core, but it would for adapters that use the library which will hopefully become more and more common in time.
src/auction.js
Outdated
| } else if (!bidResponse.vastUrl) { | ||
| logError('videoCacheKey specified but not required vastUrl for video bid'); | ||
| addBid = false; | ||
| if (FEATURES.VIDEO) { |
There was a problem hiding this comment.
this should be unnecessary, if all the calls into this are skipped the function should already be removed as dead code.
src/prebid.js
Outdated
| @@ -995,21 +998,23 @@ $$PREBID_GLOBAL$$.getHighestCpmBids = function (adUnitCode) { | |||
| * @alias module:pbjs.markWinningBidAsUsed | |||
| */ | |||
| $$PREBID_GLOBAL$$.markWinningBidAsUsed = function (markBidRequest) { | |||
There was a problem hiding this comment.
It may be just a stylistic difference, but I think it'd be better to not have this method rather than have it silently do nothing.
|
@dgirardi Thank you for the feedback and suggestions. I've pushed some additional commits to address most of them. Regarding the video-specific logic with custom renderers, I think I agree with you that that's worth tagging as its own feature in a separate PR. |
src/native.js
Outdated
| @@ -174,7 +174,7 @@ function isOpenRTBAssetValid(asset) { | |||
| logError(`for data asset 'type' property must be a number`); | |||
| return false; | |||
| } | |||
| } else if (asset.video) { | |||
| } else if (FEATURES.VIDEO && asset.video) { | |||
There was a problem hiding this comment.
I'm not sure this is right; if you turn off video but not native, does that mean that you don't want video within native?
There was a problem hiding this comment.
Fair point. Reverted!
|
could you "unfollow" your fork of prebid.js in circleCI? (or, if you know of a better way to convince it to use the correct account, I'm all ears). |
|
@dgirardi Sorry, but I'm not sure how to do what you're asking. |
|
fixes #9421 |
Type of change
Description of change
Adds a
"VIDEO"feature flag that can be used to remove code specific to video ads atcompile time. This PR includes Prebid core. We've also implemented this feature flag in a
handful of the bid adapters that we use and are prepared to submit additional PRs for those
modules.
With just the changes included in this PR we're able to shave ~4.1 KB off the size of our build
of Prebid.js...
Implementing the feature flag in the bid adapter modules obviously shrinks the bundle size further.
Other information
TESTING: We have been testing these changes in our fork of Prebid on our website for approximately 2 weeks. We have seen no significant changes in any of our metrics (CPM, CTR, viewability, etc).