Conversation
jsnellbaker
left a comment
jsnellbaker
left a comment
There was a problem hiding this comment.
Overall these changes LGTM. I wonder if we can reconsider the file name s2sVendors.js to something more generic. It's handling the s2sVendor config as well as typeParams that are used by the s2sBidders.
Additional thought, if we name it something config oriented - maybe we could move the s2sconfig stuff in there as well so that it can be easily referenced by any potential files down the road.
jsnellbaker
added
the
needs 2nd review
mkendall07
left a comment
mkendall07
left a comment
There was a problem hiding this comment.
Overall LGTM. Will want to get feedback from someone at Rubicon as well.
modules/s2sVendors.js
Outdated
| } | ||
| } | ||
|
|
||
| export const paramTypes = { |
There was a problem hiding this comment.
I do wonder if these belong here. Seems like we've split up transformation in 2 places now (in the bidder transformBidParams function and here). I do like these centralized since it's more of a problem with PBS / PBJS being in sync but wondering if they belong here or not.
There was a problem hiding this comment.
I'm not sure what the purpose is of having the config pulled out into a separate module that is then explicitly imported into the s2s module. That just means it will always be included with s2s and it might do weird things since the build process still considers a separate module (an entry point in webpack).
I'm pretty sure when I implemented the module system using webpack entry points I mentioned this as something that could not be supported in this fashion (modules directly importing other modules). However, maybe if I understand the intent better I can suggest an alternative.
|
@jaiminpanchal27 |
|
yes intent is to not have any bidder specific hardcoded stuff in prebidServerAdapter. We do not have any base factory for this so have done it like how amp does with its vendors. https://github.com/ampproject/amphtml/blob/master/extensions/amp-a4a/0.1/callout-vendors.js @snapwich What are your suggestions ? If we add it to core will it be added to build when prebidServerAdapter is not added to build ? |
|
If you include it in core it will be auto include. if you want to conditionally include it then it is correct to have it as a module; however if you explicitly require it from another module (with import) then it's no longer conditional and it will be auto-included (when you include that other module, prebidServerBidAdapter in this case). You'll need to do something like the currency module where it is included separately but then uses hooks to modify functionality, or you can do something like the express module where it exposes a function on the pbjs global and then code outside of the express module looks for it and runs it if present. |
| * | ||
| * @param {string} typeToConvert The target type. e.g. "string", "number", etc. | ||
| * @param {*} value The value to be converted into typeToConvert. | ||
| */ |
There was a problem hiding this comment.
this is missing a return type
5 participants
Type of change
Description of change
Fixes #2420