-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prebid core & PBS adapter: Feature tags and optional compilation of native support #8219
Conversation
…js' / import events from 'events.js' return different objects, which is a problem for stubbing)
@pm-harshad-mane something to consider for the pubmatic adapter? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome. the PR is a bit out-of-date now and as you stated i think there's still some things to figure out regarding testing but overall i really like this approach of using dead-code elimination as well as using the babel plugin.
really nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. One thing I am noticing while going through this (not to do with this PR but wanted to make mention of it) is that the Object.assign within PBS bid adapter is getting excessively long i.e. 500+ lines of code. We should really think about breaking this up a bit into some smaller functions to reduce complexity and make this more readable.
@mmoschovas regarding the complexity, your input is welcome in #8562 - but overall I agree :) |
@dgirardi this is released! Very exciting. Lmk when docs avail for review |
Anyone up for documenting this (awesome) feature so new bid adapters utilize it? Belongs in https://docs.prebid.org/dev-docs/bidder-adaptor.html#supporting-native |
@bretg I'll write some docs. |
…ative support (prebid#8219) * Upgrade webpack to 5; gulp build works * Fix karma, except events * Uniform access to events, import * or require (import * from 'events.js' / import events from 'events.js' return different objects, which is a problem for stubbing) * Fix (?) adapters that use `this` inappropriately * Update webpack-bundle-analyzer * Fix warnings * Enable tree shaking * Set webpack mode 'none' (or else tests fail (!)) * Update coreJS version in babelrc - prebid#7943 * Use babel to translate to commonjs only for unit tests; enable production mode * Define feature flags at compile time - starting with just "NATIVE" * Add build-bundle-verbose * Run tests with all features disabled * Fix build-bundle-verbose * Tag native#nativeAdapters * Merge master * Add fsevents as optional dep * Adjust syntax for node 12
Hey @dgirardi , these are awesome changes! One thing I noticed recently is that |
@shahinrahbariasl how are you running gulp for development? I personally found all options too infuriating to use when I first dove in, so I'm not sure what people use in the wild. I work with |
I usually do |
…ative support (prebid#8219) * Upgrade webpack to 5; gulp build works * Fix karma, except events * Uniform access to events, import * or require (import * from 'events.js' / import events from 'events.js' return different objects, which is a problem for stubbing) * Fix (?) adapters that use `this` inappropriately * Update webpack-bundle-analyzer * Fix warnings * Enable tree shaking * Set webpack mode 'none' (or else tests fail (!)) * Update coreJS version in babelrc - prebid#7943 * Use babel to translate to commonjs only for unit tests; enable production mode * Define feature flags at compile time - starting with just "NATIVE" * Add build-bundle-verbose * Run tests with all features disabled * Fix build-bundle-verbose * Tag native#nativeAdapters * Merge master * Add fsevents as optional dep * Adjust syntax for node 12
We use an webpack build with prebid as a NPM dependency. This change breaks our js bundle at runtime with
As the |
We're running into the same issue, and can't upgrade beyond version 7.1, and the error is still not handled in 7.25.
|
@wissamabyad, are you passing prebid sources through our babel configuration as in the README? |
Type of change
Description of change
This adds feature flags that can remove portions of the codebase at build time (see #7867).
The first (and only) flag defined here is "NATIVE", which is the easiest (and smallest) to manage; only core and PBS are included - with the expectation that adapters will over time mark their feature-specific code as is done here.
This is the effect on the bundle size:
A readable comparison of the output: https://gist.github.com/dgirardi/c506a2aa243125cc2a8bdcfa27e10d7a/revisions
Other information
Testing: the strategy is, for now, to add an additional test run with all features disabled. Once we have more flags defined I intend to revisit this - it will probably be too expensive to run the whole test suite on all feature combinations, but I am optimistic that we can find a good heuristic to identify "relevant" tests and be exhaustive only on those (most tests do not interact with the code being removed).
Compilation: I have decided to use our Babel plugin to resolve feature flags (as opposed to webpack's
DefinePlugin
or other options like terser arguments). The reason is that it works better for npm consumers: no change is required to keep the build working, and there's no risk of our globals (i.e.FEATURES
) clashing with their namespace.Documentation PR: TBD