This repository has been archived by the owner on Mar 25, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL; DR
These were some very boring files as there was very little code specific to video ads to be found. I took some notes on some things I found while combing through the files in ZTB-1709 which I'll include below.
Renderer.js
This file is intended to be used for providing a custom video renderer for outstream video ads which (in theory) means that you could drop it entirely from the Prebid build if you had no interest in supporting video ads. However, after the initial implementation of outstream video ad support, this feature was extended so that consumers of Prebid could provide a custom renderer for any type of ad (see issue #3231) making it not terribly reasonable or feasible to remove
Renderer.js
entirely.There is one line in the file that is specific to video:
I am legitimately unsure whether or not this is a bug. 3231 specifically states that
Renderer
should support multi-format ad units so I am suspicious that the hard-coding ofmediaTypes.video
here is a bug.The rest of the code in this file is pretty media type agnostic so assuming it's still true that custom renderers can be specified on a per media type basis there doesn't seem to be any work to be done in this file.
adServerManager.js
This file exports one function:
registerVideoAdSupport(name, videoSupport)
and according to its JSDoc:So it's an API for third party modules to extend the Prebid global. It's only usages are in third party modules (
modules/adlooxAdServerVideo.js
,modules/konduitWrapper.js
, etc.) so that checks out.Since ES6 imports & exports must be static you can't just wrap the function declaration in a conditional; the best you could do is wrap the entire body of the function in an
if (FEATURES.VIDEO)
conditional, thus turningregisterVideoSupport
into a noop if the feature were turned off. This would remove about 8 lines of code from the build: not nothing, but not much.A better solution is to leave this file alone and wrap its invocations in the
modules
folder in conditionals:Doing so should result in all invocations of the function being dropped from the final build. The exported function from
adServerManager.js
will then be flagged as an unused export making it subject to tree shaking.But does it...?
Yes! When building with only the
adlooxAdServerVideo
module included and video disabled there is a noticeable difference in the resulting bundle size:if (FEATURES.VIDEO)
conditional around call to toregisterVideoSupport
?So leaving
adServerManager.js
alone and wrapping 3 lines inadlooxServerVideo.js
in a conditional saved a whopping 3,931 bytes. Not too shabby.