Prebid Core: Library support#8527
Conversation
|
This is amazing work. Thanks a lot 🎉 |
dgirardi
left a comment
dgirardi
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I disagree only with the name choice for the first library.
Also noting that this does not completely address #7936 (which is perfectly fine - I just wouldn't want that to be closed prematurely).
| /** | ||
| * Returns the origin | ||
| */ | ||
| export function getOrigin() { |
There was a problem hiding this comment.
IMO the library should just be named 'getOrigin' - 'extraUtils' invites us to dump more unrelated stuff in here, which will eventually bring about the same problem that utils has :)
| "files": [ | ||
| "./index.js" | ||
| ], | ||
| "dependants": [ |
There was a problem hiding this comment.
in principle we should be able to calculate these automatically by looking at imports. I think webpack's chunking feature does it under the hood but I don't see an easy way to extract the graph for us to use in bundle.
We can easily add it later so no request for this PR, just noting it here in case you are familiar with that feature.
There was a problem hiding this comment.
makes sense, I'm happy to explore that in the near future when cycles free up
There was a problem hiding this comment.
👍 I could see this dependents list getting out of hand quickly.
dgirardi
left a comment
dgirardi
left a comment
There was a problem hiding this comment.
LGTM! I'll note again that this could potentially break 3rd party vendors if they're using the build output as it is now - but I don't actually know of any concrete case where that would be a problem.
dgirardi
added
the
needs 2nd review
patmmccann
requested review from
mkendall07
and removed request for
robertrmartinez
mkendall07
left a comment
mkendall07
left a comment
There was a problem hiding this comment.
Overall looks great. Just left one question. Good work solving a long standing problem with the build system!
| "files": [ | ||
| "./index.js" | ||
| ], | ||
| "dependants": [ |
There was a problem hiding this comment.
👍 I could see this dependents list getting out of hand quickly.
| /** | ||
| * Returns the origin | ||
| */ | ||
| export function getOrigin() { |
There was a problem hiding this comment.
is there any restrictions on library files? What happens if you want to import another file from the library file?
There was a problem hiding this comment.
When I initially envisioned this, the maintainer of the library would determine which files are necessary.
If users were to find the library too heavy, the maintainer could either split the library (i.e by feature or by light vs advanced) or a competitor could offer a lighter library.
There was a problem hiding this comment.
We'll probably need to cover this at a later date, with some best practices or boundaries
mkendall07
removed
the
needs 2nd review
* adds extra utils * imports from extra utils * adds libraries to .submodules * adds libraries to build process * renames extraUtils * update userId submodules list
7 participants
Type of change
Description of change
This PR allows modules to share code outside of core by introducing
Libraries. With these changes, code outside ofcorecan be shared without duplication in the build.As an example, the
extraUtilslibrary is introduced. ThegetOriginfunction from the core utils has been moved to this library, and the library is now listed as a dependency of the 3 modules that use the util.Note that when the dist is built or bundled, the
getOriginfunction is not duplicated.You can test with:
gulp build --modules=ooloAnalyticsAdapter,resetdigitalBidAdaptergulp bundle --modules=ooloAnalyticsAdapter,resetdigitalBidAdapterOther information
Helps with issue #7936
Needed for #7264
@dgirardi for review