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

[POC] - Support dependencies in modules #27

Closed
wants to merge 13 commits into from

Conversation

karimMourra
Copy link
Collaborator

This is a POC to allow modules to share code by specifying module dependencies in the webpack.conf.js
The POC includes a testVideoProvider which is only meant to be used for demo purposes, as it imports some of the constant strings and code that is intended to be shared across video providers.

This POC adds a .dependencies.json file which is meant to be a lookup for each module's dependencies.
THe dependencies then get added to the entry at build time.

Note that when the dist is built, the constant strings are no longer duplicated.

@karimMourra
Copy link
Collaborator Author

@dgirardi please let me know what you think of this approach to use dependencies

Copy link

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I like this approach, but I have several reservations:

  • running "gulp bundle" (& build, etc) should work with this, the task definitions need to be updated.
  • I think this should be considered a breaking change, because it no longer allows people to concatenate output files in the way they can now.
  • I don't think the intermediaries should be called "modules", to avoid confusion. (likewise I don't think they should live under the modules folder). They look and act differently from what Prebid calls modules. "libraries" / lib would be a better choice IMO.

Adding @snapwich for more input.

@@ -0,0 +1,8 @@
{
Copy link

@dgirardi dgirardi May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if there was only one configuration file for this, to make it harder for them to get out of sync, my vote would be for something like

"libraries": {
  "video": {
    "files": [...],
    "dependants": [
       "jwPlayerVideoProvider",
        ....
    }
  }
},

IMO it could be put in the same ".submodules.json" file we already have with some refactoring / renaming.

@karimMourra
Copy link
Collaborator Author

Hi @dgirardi the comments make sense thank you for the feedback. What do you mean by "it no longer allows people to concatenate output files in the way they can now." ; I am not familiar with output file concatenation

@dgirardi
Copy link

At the moment the build generates "prebid-core.js" and one additional js file for each module; third party vendors can take those and concatenate them to make custom Prebid bundles. I am worried that changing this system will be breaking because of that - but I suggest submitting this PR for more visibility from other reviewers, maybe we decide that's not an issue. (We don't have a better way to gauge impact other than asking).

@karimMourra
Copy link
Collaborator Author

@dgirardi I fixed the bundling. Please let me know if you believe this is ready to be merged in the main videoLibrary branch

Copy link

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready, but my suggestion is to post this separately - it will be smaller and more likely to get through a second review quickly. (Your choice though)

If you want to separate it out, there are some cases in prebid#7936 that are much smaller (compared to the video module) where this would be useful, if you want to use one as a proof-of-concept. For example, there's a utils.getOrigin function that is only used in a couple of modules.

var absoluteLibraryPath = path.join(__dirname, LIBRARY_PATH);

internalModules = getInternalModules(absoluteModulePath);
var internalLibraries = getInternalModules(absoluteLibraryPath);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right logic to use for libraries - because the dependencies on them will not work unless they are explicitly declared in .submodules.json, I think this should just return the list of libraries as it's defined in there.
Otherwise this allows inconsistencies when the library does not use index.js in the way we expect here, or the compilation of files that are not used by anyone.

karimMourra pushed a commit that referenced this pull request Jul 6, 2023
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: inna <innayare@gmail.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
karimMourra pushed a commit that referenced this pull request Jul 12, 2023
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

* update minuteMediaBidAdapter - support missing params (#29)

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: inna <innayare@gmail.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
karimMourra pushed a commit that referenced this pull request Dec 29, 2023
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

* update minuteMediaBidAdapter - support missing params (#29)

* RIseBidAdapter: support currency (#35)

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: inna <innayare@gmail.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
karimMourra pushed a commit that referenced this pull request Dec 29, 2023
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

* update minuteMediaBidAdapter - support missing params (#29)

* add currency param to bid object and tests

* update getFloor function and tests

* adding test to currency param

* adding doc & currency bidfloor support & update tests

* update currency test

* remove default test

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: inna <innayare@gmail.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
karimMourra pushed a commit that referenced this pull request Mar 6, 2024
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

* update minuteMediaBidAdapter - support missing params (#29)

* support gpp for minutemedia adapter

* removed spaces

* removed extra character

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: YakirLavi <73641910+YakirLavi@users.noreply.github.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
Co-authored-by: Inna Yaretsky <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants