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

craftBidder support imRtdProvider #12459

Closed

Conversation

crumbjp
Copy link
Contributor

@crumbjp crumbjp commented Nov 18, 2024

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Read the data set by "imRtdProvider" and add it to the bidRequest.

Other information

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

@crumbjp crumbjp changed the title Craft bidder support im rtd provider Craft bidder support imRtdProvider Nov 18, 2024
@crumbjp crumbjp changed the title Craft bidder support imRtdProvider craftBidder support imRtdProvider Nov 18, 2024
@crumbjp
Copy link
Contributor Author

crumbjp commented Nov 18, 2024

The duplicated code detected here is actually a copy of our code. Additionally, it's difficult to modify the code of other bidders. What should we do in this case?

@patmmccann
Copy link
Collaborator

The rtd adapter modifies the bid request, why read from local storage?

@patmmccann
Copy link
Collaborator

The duplicated code detected here is actually a copy of our code. Additionally, it's difficult to modify the code of other bidders. What should we do in this case?

You're welcome to modify their adapter and yours to import from a common location in /libraries

@patmmccann
Copy link
Collaborator

patmmccann commented Nov 18, 2024

Interesting that you're not affiliated, their adapter certainly appears to be based on yours. With the newbid function being so different, it would seem quite difficult to do the imports of interpret response.

@crumbjp
Copy link
Contributor Author

crumbjp commented Nov 19, 2024

You're welcome to modify their adapter and yours to import from a common location in /libraries

OK, I'll do it if necessary.

The rtd adapter modifies the bid request, why read from local storage?

What we need is not the "im_segments" set from "__im_sids," but the value of "__im_uid." Therefore, ideally, we would like to modify the imRtdProvider.

However, since each bidder and module within the system likely reflect the intentions, circumstances, and responsibilities of their respective organizations, we prefer to only make changes within the scope of responsibility we can manage within our own business domain.

As someone involved with craftBidder, I believe there might be issues with modifying imRtdProvider.
I was thinking the same about the adrelevantisBidAdapter as well.
What do you think?

@crumbjp
Copy link
Contributor Author

crumbjp commented Nov 19, 2024

Currently, we are instructing media that want to use imRtdProvider as follows.
It's very complicated.

pbjs.setConfig({
  realTimeData: { // imRtdProvider
    auctionDelay: 5000,
    dataProviders: [{
      name: 'im',
      waitForIt: true,
      params: {
        cid: imcid,
        overwrites: {
          craft: function(bid, data, utils, config) { // "data" only contains im_segments.
            bid.params.imuid = window.localStorage.getItem('__im_uid');
          }
        }
      }
    }]
  },
});

@crumbjp
Copy link
Contributor Author

crumbjp commented Nov 21, 2024

Since the PR was submitted by the person in charge at Intimate Merger, this is no longer necessary. I will close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants