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

Considering a "consumer" version of npm package #5287

Open
khatibda opened this issue May 21, 2020 · 12 comments
Open

Considering a "consumer" version of npm package #5287

khatibda opened this issue May 21, 2020 · 12 comments
Assignees
Labels

Comments

@khatibda
Copy link
Contributor

We just transitioned from building an offline bundle of prebid.js (using cloned repo) to using prebid via npm, and it's almost the best thing since sliced bread. But I have some questions / feedback on the process.

The current npm process feels geared towards "developer" and opposed to "consumers" (e.g. publishers). Bet let's walk through the details in case I'm missing something.

First, we've include prebid in our own package.json. Then in our app we have something like this:

`// import the pbjs global. the version pulled is determined in package.json
import pbjs from 'prebid.js';

// register the desired modules/adapters
import 'prebid.js/modules/aolBidAdapter';
import 'prebid.js/modules/appnexusBidAdapter';
import 'prebid.js/modules/consentManagement';
import 'prebid.js/modules/consentManagementUsp';
import 'prebid.js/modules/ixBidAdapter';
import 'prebid.js/modules/oneVideoBidAdapter';
import 'prebid.js/modules/openxBidAdapter';
import 'prebid.js/modules/prebidServerBidAdapter';
import 'prebid.js/modules/pubmaticBidAdapter';
import 'prebid.js/modules/rubiconBidAdapter';
import 'prebid.js/modules/spotxBidAdapter';
import 'prebid.js/modules/telariaBidAdapter';
import 'prebid.js/modules/tripleliftBidAdapter';
import 'prebid.js/modules/userId';

// not in prebid docs, but this seems required and is consistent with prior custom bundled code
pbjs.processQueue();`

Minor feedback, but maybe your README.md could show the npm example with "import pbjs" instead of "import prebid", since you can define the global any way you want on import, so might as well be consistent with the default code publishers already likely have implemented in their app (pbjs.requestBids, etc).

Also, in our particular case, we weren't able to fetch bids until we called processQueue after import. This wasn't documented, but we tried it because we saw the same thing at the end of the prebid bundle we built offline (repo clone). I'm not sure why, but it worked once we did that.

Finally, in the case of a consumer, there doesn't need to be an expectation that people will be modifying the code base. In this case, can we just have access to pre-transpiled modules? Right now we have to include a webpack babel loader rule for prebid.js but this seem out of the ordinary. This is not how most of our other node package includes work. E.g. we just import React, and React is smart enough (looking at environment variables) to build as a dev option with more debug or as a prod min version. We don't have to transpile react ourselves. Same goes for most node modules, which is why people tend to ignore node modules folder in their babel loader rules.

Consumers should be able to include prebid core and then cherry pick which modules to include by specific imports, just as we have done above. For example, this is how we are able to import lodash/sortBy only, without pulling in the whole directory. In this manner, consumers can still include only what they need in their bundle. Separately there could be a modules.json interface or packages.json interface if you wanted to let publishers define inclusions or rules in a different way. But i think the cherry pick import could work.

I feel like if we were able to get past the raw transpiling step (and process queue), and just have people include prebid in package.json and then import as needed, this would open up a super fruitful and optimal approach for incorporating prebid into a publisher's stack.

Thoughts?

@khatibda
Copy link
Contributor Author

cc @mkendall07 as per discussion

@bretg bretg added the feature label Jun 1, 2020
@patmmccann
Copy link
Collaborator

I think the discussion in committee was around the popularity of this change to the build process. My assessment so far chatting with others is that we may have underestimated the popularity and this is actually quite useful. Will raise in the pub committee on June 4

@khatibda
Copy link
Contributor Author

khatibda commented Jun 2, 2020

great, fingers crossed!

@robertrmartinez
Copy link
Collaborator

Assigning to @patmmccann for visibility.

@patmmccann
Copy link
Collaborator

is this actually a duplicate of your proposal on #2949 @snapwich ?

@snapwich
Copy link
Collaborator

@patmmccann yeah, it's something I've suggested in the past. There's some considerations to consider if we did this though, such as what target we compiled for, updates to the build/publish process, and then still making the source files available on npm as well for those that wanted to compile their own build.

@khatibda

// not in prebid docs, but this seems required and is consistent with prior custom bundled code
pbjs.processQueue();`

This is in the README.md file. I wasn't aware consuming prebid from npm was documented on the docs site, if it is you should make a pull-request against that repo with the changes you're suggesting. I personally only documented this in this repo's README.md because I figured that's where most developers would look.

@khatibda
Copy link
Contributor Author

yep i see it in the README.md file now, thx for pointing it out. all good on that front.

@jimaek
Copy link

jimaek commented Nov 24, 2020

As a note jsDelivr also added support for modules https://www.jsdelivr.com/esm which could be helpful here

@gglas
Copy link

gglas commented Mar 1, 2021

@khatibda do you have a handy link to the react NPM that we could review? I think we're open to doing some work here -- but want to make sure we don't clutter our NPM implementation on accident in the course of trying to set it up. Maybe we could discuss on the JS meeting this week?

@khatibda
Copy link
Contributor Author

khatibda commented Mar 1, 2021

@gglas i'm assuming you mean a link to this, but lemme know if otherwise:
https://www.npmjs.com/package/react

when is the JS meeting? i usually attend pub and video, but haven't attended JS before

@dgirardi
Copy link
Collaborator

dgirardi commented Mar 11, 2022

I plan to approach this by making both minified and unminified Prebid.js available in npm, so the "consumer" would do:

import 'prebid.js/dist/dev/prebid-core.js';     // or 'prebid.js/dist/min/prebid-core.js'
import 'prebid.js/dist/dev/someBidAdapter.js'   // or 'prebid.js/dist/min/someBidAdapter.js'
// etc

I think we should offer the choice between minified or not, rather than attempt to sniff that from the environment, as that's going to depend on the build tools in use - and unlike React we are not attempting to offer any additional integration with those.

@dgirardi
Copy link
Collaborator

Until #9333, I never realized that when pulled in as an npm dependency, it's possible to access all exports - 99% of which are not meant to be public. By allowing this we are inviting pubs to write logic that's very likely to break through updates.

I think we need a better approach than just making compilation results available; we should have an additional translation step that generates an npm package exporting only what is now available under the pbjs global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

8 participants