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

Unable to use spread operator/Set object #1892

Closed
jsnellbaker opened this issue Nov 28, 2017 · 8 comments
Closed

Unable to use spread operator/Set object #1892

jsnellbaker opened this issue Nov 28, 2017 · 8 comments

Comments

@jsnellbaker
Copy link
Collaborator

@mkalafior

Within the recent updates for Justpremium 1.0 adapter (see #1881), the spread operator and the Set object are used within the BuildRequests method. We recently discovered this type of implementation is causing issues in certain scenarios.

Can you remove these references to spread/Set and implement an alternative approach that would still suite the needs of that part of the code?

@mkendall07
Copy link
Member

FYI will need to remove your adapter by tomorrow if this isn't resolved to get to a clean build

@mkalafior
Copy link
Contributor

@jsnellbaker i would be grateful if you can explain what exactly is the problem or just point to some examples.

@mkendall07 I will update the code today. No need to remove our adapter :)

mkalafior added a commit to mkalafior/Prebid.js that referenced this issue Nov 29, 2017
@mkendall07
Copy link
Member

Thanks for the quick response!

@jsnellbaker
Copy link
Collaborator Author

@mkalafior
As part of our build process, the code is ran through a transpiler so that it it's able work across a variety of browser versions. These particular spread/set operators were throwing several errors during our testing process of the transpiled code and were preventing the build from going through; it seems these operators are not supported in IE and Safari browsers.

Below is the only line of code I found in the adapter that would need to be adjusted:

buildRequests: (validBidRequests) => {
...
const payload = {
      zone: [...new Set(validBidRequests.map(b => {

I hope this helps.

@mkalafior
Copy link
Contributor

@jsnellbaker thanks for explanation. I need to say that I'm surprised that transpiler is not able to handle this. We use such construction in multiple project transpiled by webpack + babel and never noticed any problems.

Regarding the code highlighted by you, the fix for it was sent in #1895.

@mkendall07
Copy link
Member

@mkalafior
This is a bit of a hot topic but basically the reason you can't use a number of ES6 constructs is because of requiring the use of poly-fills that pollute the global namespace. This is not something we think Prebid.js should be doing - as it's dropped into various different environments and we don't want to cause side effects. Additionally this adds considerable weight to the build.

so we make the best decision for trade off of usability of ES6 vs compiled library size. Hope that helps clarify.

@mkendall07
Copy link
Member

Also - we are exploring ways to catch these issues before merges happen. We are looking at having a build pipeline into travis-ci that would catch it and fail the build. Thanks

@mkalafior
Copy link
Contributor

Thanks @mkendall07, this explains everything.

mkendall07 pushed a commit that referenced this issue Nov 29, 2017
* Justpremium adapter and unit tests.

* Fix test suit.

* Performance improvements.

* Changes requested in pull request review.

* Register justpremium adapter in adaptermanager

* pass through bid from request

* fix linting errors

* Load polyfills for older browsers

* Load polyfills if older browser

* Remove package-lock.json

* Copy new Justpremium adapter from feature/1.0 branch

* #1881 Requested changes applied

* #1892 Use `filter` instead `...new Set` to get unique values
jaiminpanchal27 pushed a commit that referenced this issue May 23, 2018
* Justpremium adapter and unit tests.

* Fix test suit.

* Performance improvements.

* Changes requested in pull request review.

* Register justpremium adapter in adaptermanager

* pass through bid from request

* fix linting errors

* Load polyfills for older browsers

* Load polyfills if older browser

* Remove package-lock.json

* Copy new Justpremium adapter from feature/1.0 branch

* #1881 Requested changes applied

* #1892 Use `filter` instead `...new Set` to get unique values

* JSD-2248 update for adapter and tests

* JSD-2248 added transactionId in json array

* JSD-2248 adapter changes

* JSD-2248 adapter changes

* Update docs

* Remove unnecessary return statement

* Support for gdpr_consent in bid adapter

* new cookie link and endpoint

* back to old endpoint version

* sending version of prebid and adapter

* sending version of prebid and adapter

* without version

* update for tests

* changes for getUserSyncs method

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

No branches or pull requests

3 participants