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

refactor (build: gulpfiles) speed up build by only included arg modules #4263

Closed
wants to merge 3 commits into from

Conversation

potench
Copy link
Contributor

@potench potench commented Oct 6, 2019

Type of change

  • Build related changes
  • CI related changes

Description of change

When a list of bid-adapters is supplied, only build prebid with those adapters. If no custom list of adapters is supplied, build prebid with all adapters.

@potench
Copy link
Contributor Author

potench commented Oct 7, 2019

I'm a bit confused by the prebid build...

gulp build 

This appears to build build/[dev|dist]/prebid.js with ALL ADAPTERS! It also copies individual adapters to the build/[dev/dist] folder presumably for import?

gulp build --modules dfpAdServerVideo,aol

This appears to build build/[dev|dist]/prebid.js with just the adapters listed. It also copies all individual adapters to build/[dev/dist] folder... again presumably for import?

Shouldn't it instead:

  1. For import use case -
  • build prebid.js with no adapters
  • copy all adapters over to build for to be individually imported
  1. For <script> loading,
  • build prebid.js with --modules
  • don't copy any adapters over; they get included with prebid.js
  1. What is prebid-core.js for? I can't find documentation on how to use it with imports?
    According to this http://prebid.org/dev-docs/modules/currency.html
gulp build --modules=currency,exampleBidAdapter

The command will build the following files:

  • build/dist/prebid-core.js - the base Prebid code
  • build/dist/currency.js - additional code for the currency feature
  • build/dist/exampleBidAdapter.js - a specified bidder adapter
  • build/dist/prebid.js - a combined file with the base Prebid core code, bidder adapter code, and the currency module code.

Prebid only appears to function when I use prebid.js, the combined file, instead of pulling these pieces together with import in a more dynamic way. Am I missing something?

@potench
Copy link
Contributor Author

potench commented Oct 7, 2019

Ok I did some more digging here and see the following

  1. The default gulp script will just build individual adapters and prebid-core to build/[dist|dev]/
yarn gulp // (default) // builds all adapters and prebid-core
yarn gulp --modules= dfpAdServerVideo,aol // only builds 2 adapters and prebid-core
  1. The bundle gulp script handles building prebid.js using pre-built files in build/[dist|dev]/
yarn bundle // will error unless all modules are already built because it wants to bundle ALL ADAPTERS.
yarn bundle --modules=dfpAdServerVideo,aol // will build a prebid.js with just these adapters

This makes more sense, but it's still not quite what I want. There should be better separation and documentation so you know what you're building:

  • yarn gulp should build and move all adapters (this works as expected)
  • yarn bundle should build prebid.js with only prebid-core.js and no adapters
  • yarn bundle --modules=dfpAdServerVideo,aol should build prebid with just these adapters
  • yarn bundle --modules=all maybe introduce a flag to build prebid.js with all adapters, I don't know who would use this? Would anyone?
  • yarn build --importModules=dfpAdServerVideo,aol --bundleModules this could yarn gulp --modules=${importModules} && yarn gulp bundle --modules=${bundleModules} so it's just a lot more clear what's happening.

Going to update my PR for the above... ☝️

@snapwich
Copy link
Collaborator

snapwich commented Oct 7, 2019

Why are you refactoring this? If I understand your goals perhaps I can help.

To answer some of your questions:

This appears to build build/[dev|dist]/prebid.js with ALL ADAPTERS! It also copies individual adapters to the build/[dev/dist] folder presumably for import?

Building with all adapters is the default. Using a prebid.js bundle like this (with all the modules) is usually only for testing/debugging purposes. Having all the built files available for bundling is for other bundling processes, prebid.js packager being the open source one, but there are closed source systems as well.

These are not for direct import as they are not published to the NPM repository, only source files are published to NPM (in which case the dependent package is responsible for building).

What is prebid-core.js for? I can't find documentation on how to use it with imports?

prebid-core.js is the combination of all the core source files. Once again, it's not for direct import.

For general context, the primary reason all source files are built and outputted is because a Prebid.js bundle can be created dynamically through concatenation which allows runtime bundling. If you were to concatenate prebid-core.js with appnexusBidAdapter.js, and add the line pbjs.processQueue() on the end and include that in a browser, it would be a working Prebid.js bundle with the appnexus bid adapter. The download page uses this for building quick downloadable bundles and other exchanges use this for creating dynamic Prebid.js bundles.

@potench
Copy link
Contributor Author

potench commented Oct 8, 2019

@snapwich thanks for the reply. I’m going to close the pull request - here’s some feedback after I spent some more time with the gulpFile yesterday.

For some background I maintain a fork with custom adapters that are not public. I publish prebid to an internal npm repository for import support and I publish bundled/hosted prebid for <script> loading support. It is used on a lot of different sites with different adapter lists from the same fork.

I thought prebid 2.x was refactored to support importing prebid dependencies instead of needing to bundle them. But maybe it was intended to support imports internally?

After a deep dive yesterday, I’m realizing prebid is webpacking core-js and adapters. This makes sense for bundles - but ultimately I want imports not bundles. The prebid bundle is 36k with no adapters, I’m wondering now how much of this is core-js polyfills that I’m double loading on sites.

IMO the build system was a bit confusing

  • lint and test all adapters ... yes, but I don’t understand why you would ever build them all by default. Lint and test don't depend on build anyways.
  • create a separate build:lib that just babels output to a /lib folder for npm
  • more clear separation in default, build, bundle because leaving the raw files in the same place as the bundled file creates confusion about their relationship. I assumed I could import the adapters and didn’t assume prebid.js had them already bundled because they are in the same output directory.

Your notes about prebid-core and processQueue are super helpful! Also thanks for the tip on prebid-packager, I didn’t know about it.

@potench potench closed this Oct 8, 2019
@mkendall07
Copy link
Member

@potench
Thanks for the feedback on the build system. Just so we don't lose this comment, would you want to open an issue with your main use case(s) and suggested changes? For example, I tend to agree that we could separate the directory for prebid.js and the individual adapter bundles make sense.

@snapwich
Copy link
Collaborator

snapwich commented Oct 8, 2019

I thought prebid 2.x was refactored to support importing prebid dependencies instead of needing to bundle them. But maybe it was intended to support imports internally?

It was, there are instructions on how to use prebid as a direct npm dependency in the README.md.

The prebid bundle is 36k with no adapters, I’m wondering now how much of this is core-js polyfills that I’m double loading on sites.

You can see how much of it is core-js and polyfills by running gulp build with the --analyze flag.

Screen Shot 2019-10-08 at 10 17 45 AM

The double-loading of polyfills is one of the primary reasons why we don't publish built files to NPM. By publishing the source files and allowing end users to import and bundle with their own webpack process, we're allowing webpack to optimize the entire bundle and de-dupe shared dependencies. We're also allowing the end user to choose not to bundle polyfills at all (if they only support modern browsers).

@potench
Copy link
Contributor Author

potench commented Oct 8, 2019

@snapwich thanks again for the responses;

It was, there are instructions on how to use prebid as a direct npm dependency in the README.md.

So, this is precisely where things get confusing, the readme says:

import prebid from 'prebid.js';
import 'prebid.js/modules/rubiconBidAdapter'; // imported modules will register themselves automatically with prebid
import 'prebid.js/modules/appnexusBidAdapter';
prebid.processQueue();  // required to process existing pbjs.queue blocks and setup any further pbjs.queue execution

prebid.requestBids({
  ...
})

The first line is importing the bundled prebid.js that already includes adapters and core-js and prebid-core.js. With the current build process there isn't a way to generate prebid.js without including all (by default) or some (using --modules arg) adapters. So the example above isn't accurate, you are importing already imported dependencies after line 1.

@potench
Copy link
Contributor Author

potench commented Oct 8, 2019

Oh, maybe prebid.js is a direct import of https://github.com/prebid/Prebid.js/blob/master/src/prebid.js? Ah, oops, I see now. Thanks

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.

3 participants