Skip to content
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

userID and adpod modules being included in prebid-core #3908

Closed
snapwich opened this issue Jun 13, 2019 · 7 comments
Closed

userID and adpod modules being included in prebid-core #3908

snapwich opened this issue Jun 13, 2019 · 7 comments
Assignees
Labels

Comments

@snapwich
Copy link
Collaborator

Type of issue

bug

Description

The userId and adpod modules are being included in prebid-core:
Screen Shot 2019-06-13 at 5 15 35 PM

This is happening because the modules are being referenced from other modules directly:
https://github.com/prebid/Prebid.js/blob/master/modules/freeWheelAdserverVideo.js#L10
https://github.com/prebid/Prebid.js/blob/master/modules/digiTrustIdSystem.js#L15

Modules should not import other modules, doing so breaks the convention we've established where each module is a separate webpack entry point and module dependencies are specified at build time with --modules.

If we want to revisit that convention and how it's implemented we can, but as it is this causes issues with the build system thinking the modules are now "shared code" and belong where shared code goes: in prebid-core and are growing all prebid.js builds by 50kb whether they use these modules or not.

@jaiminpanchal27 @idettman @bretg @mkendall07

@jaiminpanchal27
Copy link
Collaborator

I tried one approach to solve this in one of my PR here https://github.com/prebid/Prebid.js/pull/3787/files#diff-583ded51e69735951bd128c9066eca2fR31

I added a common folder inside modules which will be a place to keep all shared code for modules. Intent here is that for any publisher they should only specify what id system they want to use in --modules. userId module is the base of it and should be automatically imported by Prebid build system.

Let me know what do you think or if you have any other way to solve this problem

@goosemanjack
Copy link
Contributor

The userId system was designed with each IdSystem operating as a submodule of the userId module. In order to avoid build breaks where an IdSystem submodule is included in the build, but the userId is not explicitly included the userId gets automatically pulled in. If you don't want this included you should be able to exclude the digiTrustIdSystem to remove this dependency. So to be clear, this is expected behavior.

@snapwich
Copy link
Collaborator Author

you should be able to exclude the digiTrustIdSystem to remove this dependency. So to be clear, this is expected behavior.

I'm not sure I follow. If the modules ends up in prebid-core (like they are doing) that means they are included in all prebid.js bundles whether the digiTrustIdSystem system is included or not. Sounds unexpected considering you just described a different expected behavior.

@jaiminpanchal27 I'm checking out your code and taking a look right now. My intuition is saying this is going require either some addition to prebid-core to allow modules to communicate dependencies with each other, or some webpack hack around the current module system (seems unlikely to work, but maybe). Also, before we can even come up with those solutions, the requirements of a submodule system need to exist somewhere. Do they?

@bretg
Copy link
Collaborator

bretg commented Jun 14, 2019

@jaiminpanchal27 - agree with @snapwich here -- userId and sub-mods can't be included by default.

@goosemanjack
Copy link
Contributor

@bretg That would actually be a fairly significant behavioral change to the existing build process. Per build instructions in Readme:

The standard build output contains all the available modules from within the modules folder.

In order to exclude these modules you must specifically identify the modules you do want to include. EX:

 $ gulp build --modules=openxBidAdapter,rubiconBidAdapter,sovrnBidAdapter

https://github.com/prebid/Prebid.js#user-content-build-optimization

@snapwich
Copy link
Collaborator Author

@goosemanjack the standard build output isn't meant for production use. It's for testing and development. To get an actual production build people either: download through the download page (which is essentially an abstraction over gulp build --modules=...), include prebid-core and modules individually through npm into their application, or build their own prebid.js bundle from command line using --modules=.....

I think submodules definitely have some uses, we just need to figure out a better way to do it. I'm brainstorming some ideas right now but am open to ideas.

@stale
Copy link

stale bot commented Jun 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 28, 2019
@stale stale bot closed this as completed Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants