-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Build system: make compiled prebid-core and modules available for npm consumers #8175
Conversation
@khatibda please take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell of how this should work (from the description, etc), this seems to be working as described. As requested though, I will not merge until it's fully ready.
If you know anyone who should look at this change, feel free to add them. I'm not sure who uses npm with prebid off-hand, but they'd probably have an interest here.
@snapwich do you happen to know? I think you may have done some work on this topic in the past (if I remember rightly).
There's quite a bit of interest about this expressed in #5287, although that issue is quite old now and I'm not sure if that still applies. |
@jsnellbaker i don't know specifically who is using the npm build, sorry. |
@khatibda does this approach match your expectation for a "consumer" npm package? The usage would look like: import 'prebid.js/dist/min/prebid-core.js';
import 'prebid.js/dist/min/some-module.js';
// ...
pbjs.processQueue(); with no transpilation necessary. There is no automagical detection of production / development flags as I'm not sure how you could do that without being opinionated about the build tools in use - which is what I think React does. |
no transpiling would be magical! i'm assuming consumer npm use case is mostly a "production" case even in dev. different from contributor npm use case. but i could be wrong. regardless, at least for webpack, if both dev/prod import paths are available, there are ways to handle. most major package imports look a little "nicer" in terms of syntax used, e.g. import 'prebid/core', but i think that's probably just sugar and not a big deal if complicated to solve. |
@khatibda we're having some trouble here in that we cannot find a second reviewer. Are you able to review? |
final q, i recall the big hangup before was about dynamically defining the global variable name that happened at transpile time i don't see any commentary on this thread, but i do see the inline comment example referring to "pbjs.processQueue()" so...does this presume/mandate that the origin global is pbjs? or do i have it wrong? (which i suppose devs can reference themselves via variable reference if they really need to.) |
looks like a merge conflict for gulpfile.js as well |
@khatibda yes - the global var is always I suppose we could try to transpile into es6 modules and avoid the global completely. |
i think pbjs is great/fine. it will work better than no globals. going to approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@dgirardi this appears ready to merge with a docs commit; some changes to https://github.com/dgirardi/Prebid.js/blob/consumer-npm/README.md, and the conflict resolved. |
@patmmccann it also needs a change in the release automation scripts (and some coordination to make them happen at the same time), so keep this labeled as |
is it possible to rely on Line 9 in 97ae929
|
@patmmccann yes, but I don't think it makes sense in this context - that will always contain just |
We are using custom builds as well, but we quite like the Babel step in between as it allows us to compile different bundles depending on the targeted browser ( es6 module build and es5 with polyfills ). @khatibda getting rid of Babel in your build means you rely on prebid.js output being compatible with the devices you want your bundle to be compatible. This can come at a cost of larger JS bundles. While I think this is interesting for faster example builds, this is not something we would use for production. |
I know this is super old but is it on the roadmap to have dist build files published for NPM? I imagine it would make things a lot easier for folks importing via NPM. |
@mkendall07 it's still an open request - and I'd like to get to it - but other work has taken precedence so far. I'll close this PR (it's inadequate) to continue the dicussion in #10086 |
Type of change
Description of change
This updates the build to make compiled files (both minified and not) available under 'dist', so that NPM consumers may use them without needing to set up Babel transpilation.
Resolves #5287
NOTE: this also needs an update in the release process; please review but do not merge.