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

Appnexus Bid Adapter: support native feature flags #8597

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Feature

Description of change

Context for the underlying Feature Flags setting: #8219

Updating the appnexusBidAdapter to support the native-based Feature Flags.

@jsnellbaker jsnellbaker requested a review from dgirardi June 22, 2022 14:37
@jsnellbaker
Copy link
Collaborator Author

@dgirardi If you have some time, would you be able to review this initial changes here for our adapter to see if they're correct & proper? I reviewed the original PR and its changes, and I tried to emulate adding the new flag in the parts of the code where we perform native related steps.

From what I could tell, it seemed the flags were only needed where we were doing active changes to auction objects or calling a 'native' function (as opposed to wrapping the underlying native functions themselves where they were defined).

I know there are flags that can be placed in the unit tests, but I wanted to get some feedback on the adapter changes before going to that far.

I ran some build command tests for reference (so it seemed to work):

12033-p-mac:Prebid.js jsnellbaker$ gulp build-bundle-prod > /dev/null 2>&1 && ls -l build/dist/prebid.js
-rw-r--r--  1 jsnellbaker  staff  2382634 Jun 22 10:27 build/dist/prebid.js
12033-p-mac:Prebid.js jsnellbaker$ gulp build-bundle-prod --disable=native> /dev/null 2>&1 && ls -l build/dist/prebid.js
-rw-r--r--  1 jsnellbaker  staff  2368577 Jun 22 10:27 build/dist/prebid.js

12033-p-mac:Prebid.js jsnellbaker$ git stash
Saved working directory and index state WIP on appnexus_native_flags: b74a97fc9 Lasso bid adapter: add new bid adapter (#8503)
12033-p-mac:Prebid.js jsnellbaker$ gulp build-bundle-prod --disable=native> /dev/null 2>&1 && ls -l build/dist/prebid.js
-rw-r--r--  1 jsnellbaker  staff  2373729 Jun 22 10:28 build/dist/prebid.js

Thank you.

@dgirardi
Copy link
Collaborator

It looks good to me - and yes, the idea is to tag out use, not definitions (the latter should fail in tests). You can use build-bundle-verbose to see what happens after dead code elimination:

gulp build-bundle-verbose --disable NATIVE

result: https://gist.github.com/dgirardi/23ff993a7e09119eb32de774e8ca0caf

the only two remaining relics I see are:

  • supportedMediaTypes still contains NATIVE
  • onBidWon is still present, but empty

both should not matter, you could tag them as well but I personally think that's too much ugliness for how much you save out of it.

@jsnellbaker jsnellbaker marked this pull request as ready for review June 24, 2022 13:35
@jsnellbaker
Copy link
Collaborator Author

@dgirardi Thanks for the initial feedback. I updated the unit tests now - could you take another look? Thanks.

@jsnellbaker jsnellbaker requested a review from Fawke June 24, 2022 13:56
@ChrisHuie ChrisHuie changed the title appnexus bid adapter - support native feature flags Appnexus Bid Adapter: support native feature flags Jun 28, 2022
@ChrisHuie ChrisHuie removed the request for review from Fawke June 30, 2022 11:01
@ChrisHuie ChrisHuie merged commit bd4f220 into master Jul 6, 2022
@ChrisHuie ChrisHuie deleted the appnexus_native_flags branch July 6, 2022 11:23
bwhisp pushed a commit to bwhisp/Prebid.js that referenced this pull request Jul 13, 2022
* appnexus bid adapter - support native feature flags

* add feature flag to native tests
ahmadlob referenced this pull request in taboola/Prebid.js Jul 27, 2022
* appnexus bid adapter - support native feature flags

* add feature flag to native tests
RomainLofaso pushed a commit to criteo-forks/Prebid.js that referenced this pull request Aug 8, 2022
* appnexus bid adapter - support native feature flags

* add feature flag to native tests
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.

3 participants