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

Prebid core: improve library support; make AnalyticsAdapter a library #8599

Merged
merged 6 commits into from
Aug 3, 2022

Conversation

dgirardi
Copy link
Collaborator

@dgirardi dgirardi commented Jun 22, 2022

Type of change

  • Feature

Description of change

This:

  • improves library support to automatically generate libraries and track their dependencies, so that consumers of a library simply need to import symbols from it;
  • makes AnalyticsAdapter.js a library, so that it can be shared when multiple analytics adapters are included in the build.

Other information

Closes #7936
Original library PR: #8527
Minor docs update: prebid/prebid.github.io#3863

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Checked the build file and saw the duplicate references for the analytics adapter code only appeared once, instead of dozens of time.

So this appears to working from what I can tell. Nice work :)

@dgirardi dgirardi mentioned this pull request Jul 25, 2022
1 task
Copy link
Contributor

@mmoschovas mmoschovas left a comment

Choose a reason for hiding this comment

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

lgtm

@patmmccann patmmccann merged commit db2a333 into prebid:master Aug 3, 2022
@robertrmartinez
Copy link
Collaborator

Looks like the libraries/analyticsAdapter/AnalyticsAdapter.js formerly known as src/analyticsAdapter.js is no longer auto included in my prebid builds using same old command as before.

Our process uses Prebid Packager https://github.com/prebid/Prebid.js-packager which I am thinking is the problem.

Since src/analyticsAdapter.js is not inside the generated rubiconAnalyticsAdapter.js, prebid packager needs to include it in the build. Guessing it does not handle libraries or something.

I'll continue to look into it.

@robertrmartinez
Copy link
Collaborator

Yeah, so now looks like we need to update prebid packager to correctly include the necessary library files if modules from the build include it.

I see analyticsAdapter.js is output now after this PR when before it was not included in output files, it was just added into analyticsAdapter files themselves.

So this needs to be included in the build.

As I think we are the only ones working with prebid packager, I will try and fix this.

@dgirardi
Copy link
Collaborator Author

dgirardi commented Aug 23, 2022

@robertrmartinez the build now generates a dependencies.json that links output files to the output "library" files they need. I was unfortunately completely unaware of the packager.

@robertrmartinez
Copy link
Collaborator

@dgirardi No problem at all. It was written by @snapwich when he worked at Magnite for our system. And we still are using it!

Thanks for pointing out the deps file, that is nice and I will use it to make a PR to packager.

@robertrmartinez
Copy link
Collaborator

@dgirardi I whipped up something real quick which "solves" this. I know it could be written better / cleaner of course, but unfortunately I am slammed at the moment.

prebid/Prebid.js-packager#4

I guess we should prob get off of this prebid packager sometime maybe, but it makes our system run quicker since we do not have to run gulp build every time we build a prebid wrapper.

@dgirardi dgirardi mentioned this pull request Sep 1, 2022
1 task
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
…prebid#8599)

* Automatically resolve libraries and their dependencies

* Move AnalyticsAdapter under libraries
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
…prebid#8599)

* Automatically resolve libraries and their dependencies

* Move AnalyticsAdapter under libraries
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 this pull request may close these issues.

Bundle size inflated due to intra-module dependencies
9 participants