Modules

[snapwich]

Type of change

• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes

Description of change

The module system as described in #1086

For an example of converting an existing adapter to a module look at the adaption in file src/adapters/rubicon.js to modules/rubiconAdapter.js and tests from test/spec/adapters/rubicon_spec.js to test/spec/modules/rubiconAdapter_spec.js.

New express module was added as well. With the new express module and the conversion of the rubcion adapter to a module the build currently spits out 3 files: prebid.js, express.js, and rubiconAdapter.js. These can then being bundled for usage after gulp is run using gulp bundle --modules=express,rubiconAdapter. The --modules flag is optional if you just want to bundle all modules found.

[snapwich]

Will update this pull-request with external module support using require.resolve for npm support.

[protonate]

As discussed let's incorporate rollup and plugins to achieve this functionality while preserving & standardizing on es6 module syntax, also to minimize the required changes to adapters, tests, etc.

[snapwich]

I've added a commit (5d7ade4) showing an example of splitting modules only using webpack code-splitting. I didn't update the express module as we'll still need to figure out the best way to get config to modules, but the rubiconAdapter module has been updated.

[snapwich]

I've updated this pull-request with the rubicon, appnexus, and appnexusAst adapters converted to modules.

To test you can clone the repo and checkout the modules branch, yarn install, and then gulp which generates the built components. Follow that with gulp bundle which bundles all of the built modules (by default) into an individual file build/dist/bundle.js.

You can also use gulp bundle --modules=appnexusBidAdapter,appnexusAstBidAdapter to specify only certain modules to be included in the bundle.js file.

gulp build will do both the build step and the bundle step as a convenience and accepts the same --modules argument.

Besides searching for modules in the ./modules directory, modules are also sought in node_modules directory from within the repo, or node_modules directories in parent folders. You can specify external modules by name using the same --modules argument. e.g. gulp build --modules=someNpmPkg. These external modules are included in the build process. i.e. external modules are ran through the webpack build so they can import, use babel features, and be written as a regular module would be written from within the project.

Starting tomorrow I will begin the process of converting the rest of the adapters into modules.

Note: One final thing probably needed before being production ready is to create the webpack plugin to exclude the manifest and jsonp loading code from the webpack runtime. These bundles work fine as is implemented here, but excluding that code will reduce the bundle size.

[snapwich]

Another thing to note: in order to support supportedMediaTypes that was handled by the adapter-loader, video-loader, and native-loader; I have added an additional parameter to adaptermanger.registerBidAdapter that supports a configuration object that includes supportedMediaTypes

[protonate]

@matthewlane @jaiminpanchal27 @dbemiller added all of you as reviewers, please have a look as this is working towards final and will need to be understood by all of us.

ready for re-review

[dbemiller]

I looked this over... but I must admit, I think I'm too late to the party to give much serious feedback.

A build task which creates a bundle with only the adapters you need looks like a great contribution. The spec was also super clear and well-written.

I'm getting hung up on the claim: "we want dynamic runtime inclusion without async loading." The main benefit of for dynamic runtime inclusion which I can think of is the fact that you can download only the parts of the javascript which you need. Since it sounds like you still expect publishers to have downloaded it all, then what benefit is there to loading things at runtime?

On the other hand... I don't actually see anything in the diff which adds APIs to allow that. So am I missing something, or did that part of the spec get dropped?

The rest is mostly details. Will leave some comments on individual files about them.

[dbemiller]

Could you leave comments at the top of new files describing (at a high level) what their purpose is?

It's helpful for a few reasons... but those are even more pronounced now that we're making Modules.

• It helps newcomers get their bearings on the codebase
• It helps users decide whether or not they should include the module (at a glance) in their bundle
• It (theoretically) improves code quality over time, because some future engineer tasked with implementing "feature X" can tell at a glance which (if any) code makes sense in this module.

[dbemiller]

This looks accidental... are you using jscs anywhere? I thought we got rid of that with the move to eslint.

[snapwich]

I added the gulp-if but am not sure what gulp-jscss deal is. If it was removed I might have accidentally re-added it after a merge conflict, I can remove it if its no longer needed.

[dbemiller]

yeah, it was removed here in #1111

[dbemiller]

Does this replace src/adapters/adapter.js? I don't see that file being deleted... but they're duplicates.

[snapwich]

yes, this replaces src/adpaters/adapter.js. I haven't removed the old file yet because I haven't finished converting all the adapters to modules yet, so the non-converted adapters still look for this file. As soon as I'm done converting the adapters to modules I'll make sure this is moved properly in git.

[dbemiller]

What happens if getConfig is called after the last call to setConfig? If I were calling these APIs, I would still expect my callback to be executed.

Actually... a related problem: I would expect my callback to be executed exactly once. It doesn't affect your use-cases here... but looking at this file in isolation, I'd still consider it a bug.

Related: are unit tests coming soon?

[dbemiller]

this looks..... well, very dangerous. By setting a variable in an async callback, you've inherently created a race condition. This works (I would guess?) because a user will (should?) call setConfig sometime between the time that this code runs, and when they kick off an auction... but if someone tried to use ACCOUNT_ID in this file outside of those auction-related functions, you could have some really tricky bugs to track down.

Part of me wants to say "your adapter, your business :p" but.... given that you're prototyping an architecture here, I'm a bit concerned because I can't really see a way to do it safely.

It seems to me like it would have to look something like:

getConfig('myNamespace', config => {
  var property = config.myParam;
  module.exports = stuffWhichUses(property);
});

...which I'm pretty sure modules don't actually let you do.

Can you write an example of a very small module which reads a value from the config, and then exports something useful once the param has been read? Without assuming that callers follow that implicit import files, setConfig, callUsefulMethod order.

[snapwich]

I don't know if I'd call it a race condition, but there is definitely a dependency problem. However, I didn't create it. The problem is inherent in the way we execute pbjs.que blocks and when module code is ran by webpack. I detailed this in a diagram I pasted to slack awhile back to describe our problem:

We effectively have a circular dependency based on the current architecture of Prebid.js. This isn't a new problem, it's just now more obvious because this is the first time someone has attempted to apply global config (config across adUnits and auctions) to an adapter (or in this case now, a module). Currently configuration can only come in through bid.params which makes this condition less obvious, but is also not an ideal solution for handling global config.

Using a callback was the solution I found to solve this. By registering a callback that sets the config it allows us to defer configuration until after setConfig is called. I think the dependency is intuitive (that you must setConfig for a module before you can use that config), so I'm not too worried about the implications.

I originally tried to solve the problem with promises, but then I ran into an actual race condition when the scheduled config callback wasn't called until after the auction had ran.

It's possible to not register the adapter to Prebid.js until the config has been received (the registration being the real export that Prebid cares about, module.exports is only used for testing), but then you're just swapping a set of config error messages, for a more cryptic and general error message. i.e. "Rubcion Adapter missing required config Account ID" vs "Rubicon Adapter was not found".

[dbemiller]

Yeah, that makes sense. I also agree that there's an architectural problem with prebid right now, and your diagram distills it down well.

One obvious fix that jumps out at me is if we moved towards an API like requestBids(config) instead, and then implement write requestBids to forward the config through to any adapters. That would also move us one step closer to multiple auctions, since it's clear what the config for each particular auction is.

I know fixing all that is outside the scope of this PR... but I wonder if we could get things a little closer to that ideal, without too much effort?

For example, right now your modules look like:

getConfig();
adapter = makeAdapter(config);
adaptermanager.registerBidAdapter(adapter, ...);

Instead, we could do something like:

function adapterLoader(config) {
  return makeAdapter(config);
}
adaptermanager.registerBidAdapterLoader(adapterLoader, ...);

Then load the config inside registerBidAdapterLoader, and forward the call through to registerBidAdapter in the config callback.

Main benefits being:

• The circular dependence stays within prebid-core, and out of the modules
• Adapters would be easier to unit test (can send through mock configs)

Another benefit (disjoint from the first... so take it as an independent suggestion) is that you could namespace the global config. For example, if the bidder code is rubicon, then registerAdapterLoader function could decide (universally) that rubiconAdapter is where your config data goes. Then when it actually executes your load function, it can send that data right back to you.

Benefits there being:

• Lower publisher learning curve (config naming conventions consistent across all adapters)
• Adapters can't trample each others' config values accidentally

[snapwich]

So am I missing something, or did that part of the spec get dropped?

@dbemiller I wrote a longer response, but for whatever reason github posted it twice and when I deleted the extra one github decided to delete both of them.... It might still be in your e-mail, but if not, here's the gist of it:

The requirement of dynamic runtime inclusion was modified in a follow-up meeting concerning modules that happened after the spec was written. It was stressed that in an effort to keep es6 module syntax we should get rid of the runtime requirement. We still want quick dynamic inclusion though. And by that I mean, concatenating built files together to include required functionality (a process which is fast and could be part of a server-side request), or including them individually like:

<script src="./prebid.js"></script>
<script src="./rubiconBidAdapter.js"></script>
<script src="./appnexusBidAdapter.js"></script>
<script>
pbjs.processQueue();
pbjs.que.push(function() {
  ... stuff here
});
</script>

[GLStephen]

@snapwich The latter (individual scripts) approach would likely see benefits in maintenance and performance in an HTTP2 environment if managed properly. In terms of caching and config and in terms of delivering from a central CDN while also allowing modularity with a known, off the shelf for everyone, mechanism: HTML.

[snapwich]

@GLStephen I think it's a feasible approach. However, I don't think it'll be used often due to the rampant use of tag managers within the ad space and the hard requirement that the prebid.js code (that contains the webpack runtime) be loaded first (so other modules have something to register with).

For that reason I think the server-side concatenation of the bundle will be more common. I think that is still a very useful addition to Prebid.js though. It'll allow us to do things like:

<script src="./prebid.js?modules=rubiconBidAdapter,appnexusBidAdapter"></script>

and get exact bundles of only the code we want on request.

[dbemiller]

I agree that server-side or build-time concatenation generally makes much more sense than multiple script tags on the page.

If you do want to leave that pattern open as a possibility, then the relevant parts of each module in this code (e.g. the registerBidAdapter code) should be wrapped inside the cmd. Otherwise you'll be forcing people to load prebid-core synchronously.

But that's pretty convoluted and icky if loading individual modules is a non-goal.

[protonate]

LGTM!

[navneetpandey]

Please note that this document http://prebid.org/dev-docs/bidder-adaptor.html now contains many broken link.

[bretg]

Thanks @navneetpandey - covered in prebid/prebid.github.io#275. May have missed some -- review welcome.

[navneetpandey]

Seem consistent now @bretg . Thanks.

[jeremiegirault]

Hi, as of this day, the RequireEnsureWithoutJsonp.js has no effect. The PR #1785 references the chunk function name for some reason, but I don't think it works as it should. Worth fixing ?

[snapwich]

@jeremiegirault what do you mean, it has no effect? I just pulled the latest and built prebid and it still seems to be successfully stubbing the jsonp loader code.

[jeremiegirault]

Hi, In my tests I had troubles and the plugin was not active indeed. But after using the good tag/master looking at the generated code it indeed seem to replace the requireEnsure function :

/******/ 	__webpack_require__.e = function requireEnsure(chunkId) {
/******/ 		if(installedChunks[chunkId] === 0)
/******/ 		  return callback.call(null, __webpack_require__);
/******/ 		else
/******/ 		  console.error('webpack chunk not found and jsonp disabled');
/******/ 	};

However callback does not exists. Indeed the prototype of requireEnsure used to be function requireEnsure(chunkId, callback). Now it returns a Promise (probably due to some internal webpack changes?).

Looking at the source without the plugin :

/******/ 	__webpack_require__.e = function requireEnsure(chunkId) {
/******/ 		var installedChunkData = installedChunks[chunkId];
/******/ 		if(installedChunkData === 0) {
/******/ 			return new Promise(function(resolve) { resolve(); });
/******/ 		}
/******/

...

So I question the usefulness of this webpack plugin which seem outdated in the context ?

[snapwich]

@jeremiegirault yeah that code is probably useless. It was an attempt to fail nicely if JSONP loading was attempted, but I don't think it's ever running. I added a pull-request to just remove that code block entirely.