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

ixBidAdapter.js: allow siteId param to be number #2729

Merged
merged 3 commits into from
Jun 20, 2018
Merged

Conversation

homeyjd
Copy link
Contributor

@homeyjd homeyjd commented Jun 14, 2018

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

Type of change

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

Description of change

This is a backwards-compatibility change only for v0.x => v1.x transition.

The v0.x adapter checked existence of the "siteID" parameter, which has been renamed to "siteId". The IndexExchange migration documentation does not include a reference to restricting the var-type of this parameter, it only mentioned a change in name. To be consistent with the documentation, it must also allow numbers.

Other information

  • Not sure who is maintainer of IX adapter. *

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.
@mkendall07
Copy link
Member

looks like you have 2 failing test cases. Please resolve. Thanks

@homeyjd
Copy link
Contributor Author

homeyjd commented Jun 15, 2018

@mkendall07 Thank you for your patience. Tests corrected.

@harpere harpere self-assigned this Jun 20, 2018
@harpere harpere added the LGTM label Jun 20, 2018
@ix-prebid-support
Copy link
Contributor

@homeyjd @mkendall07
Having it as string ensures that the number does not get converted into a scientific notation (i.e. 16000 becomes 16e3) in the scenario where config ends up getting minified. Therefore it's much safer to have it as string

@harpere harpere merged commit ae287c3 into prebid:master Jun 20, 2018
@homeyjd
Copy link
Contributor Author

homeyjd commented Jun 21, 2018

@ix-prebid-support Thank you for the context, good point.
This PR arose simply because the type-requirement was missing from the migration docs.

a) add type req to docs, reverse this PR, inform advertisers
b) add if (typeof bid.params.siteId === 'number') bid.params.siteId = bid.params.siteId.toFixed(0) in new PR
^^^ Preference?

@ix-prebid-support
Copy link
Contributor

Our preference would be to go with option a in this case.

@homeyjd
Copy link
Contributor Author

homeyjd commented Jun 21, 2018

@ix-prebid-support Sounds good. I will change my site's configuration to use strings only in anticipation of your changes.
The commit to reverse is ae287c3 whenever you're ready to submit a new PR:

@ix-prebid-support
Copy link
Contributor

@homeyjd

After some discussions internally, we've decided to keep the support for string as well as number. However, we'll always recommend in the docs to set it as string.

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
florevallatmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Sep 6, 2018
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
* ixBidAdapter.js: allow siteId param to be number

In v0.x, the siteID param would be string or number. Somehow, this was restricted to just a string in v1.x.

* ixBidAdapter.js logical error (not enough coffee)

* ixBidAdapter_spec.js: allow number for backwards compat
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