-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
IX Bid Adapter: POST Support #9032
IX Bid Adapter: POST Support #9032
Conversation
features: toggles | ||
} | ||
if (storage.localStorageIsEnabled()) { | ||
storage.setDataInLocalStorage(LOCAL_STORAGE_FEATURE_TOGGLES_KEY, JSON.stringify(this.featureToggles)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it useful to read these values from local storage instead of relying on configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabling post is dependent on our server response for the moment. We are reading from an internal config and local storage when available. The advantage of reading from local storage is we can capture requests on page load rather than waiting for a response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patmmccann let me know if you have any additional questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting ix_features: {"expiry":,"features":{"pbjs_enable_post":{"activated":true}}}
in local storage and giving a bidder an override to access local storage to enable post method seems to be a bizarre workflow for a publisher and also seems undocumented. I feel like I must be missing something here. Why couldn't the publisher opt into IX post mode via a more standard method?
We're attempting to minimize bidder access to local storage and make it a rare event, not standard practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't be relying on local storage to activate POST. Local storage access will allow us to generate POST requests on page load but this is not a dependency and is more of a nice to have.
If reading from local storage is not allowed then POST will be limited to ad refreshes on the page (since pbjs_enable_post flag is dependent on our server response). In a future PR, we will be moving completely to POST. The intention here is to roll out POST within our adapter gradually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unsatisfying reason; we'll discuss in committee. I recommend you plan on this not being approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the use of local storage the concern here? If so, would refactoring to remove the usage of local storage help address those concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lksharma
While I think the overall code looks fine (baring Patrick's feedback about the localStorage question), I was hoping to test these changes to verify the POST request completes successfully.
Do you have some sample params I can use that should invoke this state? I was trying with some of the values in the md file and hardcoding some values in localStorage, but I'd rather test with the expected natural workflow, if possible. Please let me know when you have the chance.
Thanks.
Thanks for looking into this. You can test the code flow with the following local storage value:
Let me know if you need anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @lksharma
With that in place in the localStorage (and the bidderSettings storageAllowed setting enabled), the logic presented here appears to be working fine.
Will wait on merging for Patrick's feedback.
Generally, this use of local storage is not a rules violation, but it feels bizarre and difficult to explain to publishers, who will have to grant you access to local storage one by one. I recommend retooling to avoid these conversations and to give publishers a simpler way to enable post mode. Since Prebid 7 was released, your adapter will not have access to local storage by default. |
spoke with Luke at IX, merging The use of local storage is not critical to the server side control of get vs post, but enhances it. The feature will still work for publishers who don't make an exception. |
Co-authored-by: Love Sharma <love.sharma@indexexchange.com>
Co-authored-by: Love Sharma <love.sharma@indexexchange.com>
Type of change
Bugfix
Feature
New 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
This release supports the switch from GET to POST requests via features