Bundle size inflated due to intra-module dependencies

[dgirardi]

Type of issue

Bug

Description

Prebid bundles are generated by concatenating "prebid-core.js" with a user-selected list of module files, which contain all dependencies of the module that are not in core.

Since there is no real dependency tracking beside "core" or "not core", dependencies can be repeated across module files (if they import from each other, or if they import the same non-core file).

Fixing this requires coordination with external bundle vendors.

Steps to reproduce

gulp build-bundle-dev --modules "adlooxRtdProvider,adlooxAnalyticsAdapter,id5AnalyticsAdapter"

Generated prebid.js: https://gist.github.com/dgirardi/86947f196a25fc753fbb6403094b97fe

• AnalyticsAdapter definition is repeated at lines #14737, #15830, #16447
• adlooxAnalyticsAdapter module is repeated at lines #14377, #15470

Problem persists in minified bundle, just harder to spot:

gulp build --modules "adlooxRtdProvider,adlooxAnalyticsAdapter,id5AnalyticsAdapter"

Generated prebid.js: https://gist.github.com/dgirardi/c3bb01de917407746950ec597cefbab2

• "bundle" (from here) appears 3 times
• window.pbjs.installedModules.push("adlooxAnalyticsAdapter") appears twice

[snapwich]

having duplicate code show up in a prebid.js bundle is not necessarily a sign of a problem (and with analytics adapter it's not, but not sure about adloox case). the issues you've identified are known and were addressed with what i think are acceptable solutions:

the AnalyticsAdapter is intentionally not pulled into the primary prebid-core.js entry point (where most shared code under /src ends up). the reason for this is some user bundles do not use analytics adapters (probably most) so for those that do not it would just be dead code; and those that do use analytics adapters only include one of them. so the only way you would end up with multiple AnalyticsAdapter code blocks is if you included multiple analytics adapters in your bundle which you will most likely never do. you can see this exception intentionally addressed here in the webpack config.

as for adlooxAnalyticsAdapter, prebid modules should never import code from another prebid module to prevent what you're seeing. there is actually a custom lint plugin that enforces this: https://github.com/prebid/Prebid.js/blob/master/plugins/eslint/validateImports.js#L25 but you can see here that the lint rule was disabled which allowed this to happen. so it's possible this is a bug (you'd have to dig into the PR and/or ask reviewers/submitter why this was allowed and if it was intentional).

even though modules can't directly import other modules using es6 imports it is possible for modules to have dependencies on other modules but it is handled through the module/submodule pattern (which came up years after the prebid module system was implemented so i don't necessarily think it's the greatest solution and kinda hacky). you can see an example of this with the userId module:

Prebid.js/modules/userId/index.js

Line 862 in d8282ec

module('userId', attachIdSystem);

and here's a submodule depending on it:

Prebid.js/modules/imuIdSystem.js

Line 151 in d8282ec

submodule('userId', imuIdSubmodule);

there are several userId submodules that depend on the userId module as well as other module/submodule relationships defined here. while this does allow modules to depend on each other i'm not a huge fan because: it's a custom-coded dependency that's hidden to tooling such as IDEs, it's resolved at runtime which is not ideal, and it relies on conventions such as updating that .submodules.json file which the gulp build system uses which could introduce discrepancies keeping the file up-to-date with reality.

if the module/submodule issues are something you wanted to address during this refactor of webpack modules i think it could be beneficial, although i also understand how it might be out-of-scope.

[dgirardi]

@snapwich that makes sense (and I did not know that multiple analytics adapters are unlikely), but I also think it's unavoidable that over time this model will cause dead code to accumulate (because it relies ultimately on putting significant effort into code reviews). For example, this utility function:

Prebid.js/src/utils.js

Lines 912 to 920 in 55e7cdb

	export function getOrigin() {
	// IE10 does not have this property. https://gist.github.com/hbogs/7908703
	if (!window.location.origin) {
	return window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port : '');
	} else {
	return window.location.origin;
	}
	}

Is used only in 3 modules (ooloAnalyticsAdapter.js, resetdigitalBidAdapter.js, rtbhouseBidAdapter.js). If you're not interested in any of them, or you need multiple (depending on whether we decide that should be core or not) that is dead code in your bundle.

From a technical POV, it shouldn't be hard to fix this, but I am not familiar with how the build output is used by 3rd party tools. I am thinking we can keep it as-is for compatibility but also generate a separate build that lets webpack track dependencies between chunks and capturing that into something that 3rd party tools can use. It should be possible for example to have something like build/dist/v2/dependencies.json:

{
  "ooloAnalyticsAdapter.js": ["getOrigin.js"],
  "getOrigin.js": ["prebid-core.js"],
  "adlooxRtdProvider.js": ["adlooxAnalyticsAdapter.js"],
  ...
}

[snapwich]

so it's true that there will be "dead code" for some users depending the combination of modules and/or features you choose to use. as you identified, some modules added or use shared util functions that really are only used in a one or few modules. this can be essentially solved by a publisher making their own prebid.js build from npm src, which many do. webpack performs tree-shaking to do dead code removal and if you are directly importing your modules using es6 syntax (as you should if you're using prebid from npm) webpack should be able to determine code you're not using and remove it. i haven't tested how friendly prebid is for tree-shaking, but i assume this is a solved problem; and if it's not then the bug fix should be making prebid friendly to webpack tree-shaking.

this doesn't work for users that need to build prebid.js through concatenation though. it doesn't need to be concatenation but the requirement for these users is that they can make a custom prebid.js builds quickly (i.e. not run a webpack build which is extremely slow). an example would be a user requesting the bundle /prebid.js?modules=rubiconBidAdapter,rubiconAnalyticsAdapter and it being created by the server instantly. right now approaches like this are handled by concatenating the individual webpack entry-points together. some tooling does this as part of a build process, not an http request, so http2 isn't necessary a solution either.

the last way in which prebid is customized is through features. it's pretty obvious now that if you use prebid.js but are not doing video, you are including a lot of video code in the prebid-core.js that you are not going to use. so it's essentially "dead code". solutions to this have been discussed in a few different threads. here's one i see that is still open: #6361

these are the three use cases i think you should be considering when refactoring the build process.

[patmmccann]

fwiw, using multiple analytics adapters is not uncommon

[jimdigriz]

as for adlooxAnalyticsAdapter, prebid modules should never import code from another prebid module to prevent what you're seeing. there is actually a custom lint plugin that enforces this: https://github.com/prebid/Prebid.js/blob/master/plugins/eslint/validateImports.js#L25 but you can see here that the lint rule was disabled which allowed this to happen. so it's possible this is a bug (you'd have to dig into the PR and/or ask reviewers/submitter why this was allowed and if it was intentional).

@snapwich this was intentional and highlighted at the time in #6308 (comment) as there is no 'command bus' to let modules communicate with one another...or at least there was not at the time.

The aim is to reduce the burden on AdOps teams so they do not have to duplicate identical configuration snippets across the three Adloox modules.