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

Rakuten bid adapter #5056

Closed
wants to merge 7 commits into from
Closed

Conversation

snapwich
Copy link
Collaborator

Type of change

  • New bidder adapter

Description of change

New bidder adapter for Rakuten.

Also adds typescript types for bidder interface.

Other information

Bidder params update: prebid/prebid.github.io#1899

@mkendall07
Copy link
Member

Hey @snapwich.

Can you explain how the typescript integration works? seems like you compiled it outside of prebid and then the prebid build will just use the JS version? Interesting pattern, not sure if that's a good idea or not....

height: sb.height || 0,
creativeId: sb.creative_id || 0,
dealId: sb.deal_id || '',
currency: sb.currency || 'JPY',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default to 'USD'

@snapwich
Copy link
Collaborator Author

snapwich commented Apr 1, 2020

Added a Typescript notice to the documentation.

@mkendall07 yes that's what I did. This allows us to support types for the bid adapter interface without having to add typescript to the build process. It's possible we might want to add typescript to the build process, but that would be a backwards breaking change if we did.

I should also add that this is pretty low maintenance required for us. The only tech debt we acquire with this methodology is needing to update the bidder factory or config type files if the interface changes. That should be infrequent and be more accurate than the current JSDocs types we keep.

@jaiminpanchal27
Copy link
Collaborator

@snapwich Some questions

  1. Rakuten adapter seems very simple. Will compiled code be easy to review for complex adapters ?
  2. Which files to review for reviewers ? Both typescript and compiled file ?
  3. Some adapters use utils, storageManager. Will there be a ts file for them too ?

@snapwich
Copy link
Collaborator Author

snapwich commented Apr 8, 2020

  1. it should be, the compiled javascript is almost the same as the typescript with the types removed since the compile target is a modern verison of js. (since we'll be running through babel still compiling to modern js is no problem).
  2. the compiled .js is probably the only one that matters as it's the code we'll actually be using.
  3. there could be. type adoption can be incremental so that any imported modules that don't have type definitions can be treated as any type (so basically normal javascript). but I think it'd beneficial to type as much as the bidder interface as possible.

some other typescript options are:

  1. we just keep .d.ts types in the repo but don't allow submitters to submit their .ts code (they have to manage it themselves somewhere else).
  2. we keep types as part of a separate repo and don't allow typescript at all in this repo. I have a feeling they could get out-of-date easier but maybe not. (this is how definitelytyped works)
  3. we update the webpack build to compile the .ts files so we don't have generated code in this repo. i'd consider this a backwards incompatible change (so probably a major revision bump) as it'd require people using prebid.js through npm to also update their webpack builds as well.

I described it in a little more detail here: #5097

@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 23, 2020
@stale stale bot closed this Apr 30, 2020
@snapwich snapwich mentioned this pull request May 4, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants