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

Prebid Core: Update renderAd to use hook #7091

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Prebid Core: Update renderAd to use hook #7091

merged 2 commits into from
Jun 28, 2021

Conversation

iamnewton
Copy link
Contributor

Type of change

  • Feature

Description of change

Updating renderAd to use hook in preparation to use in a Fledge proposal test.

@jsut
Copy link
Contributor

jsut commented Jun 26, 2021

It seems like this could replace #7059. Hooks seem to be more flexible than the event system is, but perhaps there are trade offs I am not aware of?

@robertrmartinez
Copy link
Collaborator

@jsut

I think both make sense.

Adding and using this hook delays the execution of the renderAd function until your piece of code finishes and calls it directly. This hook stuff is mainly used to alter and change things before the hooked function executes.

The Event Library is more for notifying, and is non blocking. So I think both have a place, and AD_RENDER_SUCCEEDED seems like a valid event (Since there are times when a BID_WON Does not equal a Ad Successfully rendering)

@robertrmartinez
Copy link
Collaborator

@iamnewton Will you pull latest master, there was a fix for a flaky test merged.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Looks fine.

I would like @snapwich to give his input since the hook library is his baby.

Rich, what are the impacts of adding this hook in the scenario where 99.9% of prebid wrappers will not actually register a hook.

What kind of overhead is there?

* master:
  Send screen size instead of browser size (#7110)
  Prebid Analytics Manager: extend mocha testing timeout to fix flaky ie test (#7104)
  IX Bid Adapter: Use openrtb imp[].banner.format to build requests (#7023)
  Sharethrough Bid Adapter: add support for GPID (#7089)
  VLYBY Bid Adapter: add new bid adapter (#6417)
  Fix the renderer tests so they can be run on their own (#7070)
  akamaiDAPIdSystem: add new ID submodule (#7084)
  Real Vu Analytics Adapter: update addUnitById() return value (#7088)
  rubicon adapter: segtax change (#7098)
  Pix Fixture Bid Adapter: add new bid adapter (#7069)
  Adkernel Bid Adapter: renaming converge alias (#7097)
  Dmd Id System: add rest endpoint (#7066)
  Added Media.net RTD Module (#6988)
  temporary dependency change and update a test eslintrc rule (#7094)
  increment pre version
  Prebid 5.2.0 Release
@snapwich
Copy link
Collaborator

very little overhead. this should be fine.

as for the event, hooks don't replace events; if you need the event i'd add that as well.

@jsut
Copy link
Contributor

jsut commented Jun 28, 2021

@snapwich @robertrmartinez thanks for the context.

@robertrmartinez robertrmartinez self-requested a review June 28, 2021 20:34
@robertrmartinez robertrmartinez changed the title feat: update renderAd to use hook Prebid Core: Update renderAd to use hook Jun 28, 2021
@robertrmartinez robertrmartinez merged commit 9ffe6f1 into prebid:master Jun 28, 2021
@iamnewton iamnewton deleted the feat-add-renderAd-hook branch June 28, 2021 20:37
prebidtappx pushed a commit to prebidtappx/Prebid.js that referenced this pull request Nov 15, 2021
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.

5 participants