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

Use package dependencies for ES6 Array shims #962

Merged
merged 1 commit into from
Feb 8, 2017

Conversation

outoftime
Copy link
Contributor

@outoftime outoftime commented Jan 31, 2017

The shims that are vendored in to the library break in IE; also, no reason to reinvent the wheel. Instead, use the array.prototype.find and array-includes modules to ensure compatibility.

Original error message in IE:

The shims that are vendored in to the library break in IE; also, no
reason to reinvent the wheel. Instead, use the `array.prototype.find`
and `array-includes` modules to ensure compatibility.
@mkendall07
Copy link
Member

LGTM. We should figure out why our unit tests didn't catch this.

@protonate protonate merged commit 22cdd6f into prebid:master Feb 8, 2017
Walexander pushed a commit to MbidIO/Prebid.js that referenced this pull request Mar 6, 2017
The shims that are vendored in to the library break in IE; also, no
reason to reinvent the wheel. Instead, use the `array.prototype.find`
and `array-includes` modules to ensure compatibility.
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.19.0 to release/1.14.0

* commit 'b13f7ba7ee8b3c168dc0af7c8bfc94747d017e70': (34 commits)
  Add changelog entry.
  Prebid 0.19.0 Release
  check truthiness of adUnitCode (prebid#990)
  fixed exception when refreshing individual Ad Units and bidder responds slowly (prebid#989)
  Stub pixel drop to prevent network request in test (prebid#988)
  Updating Komoona adapter to support future Prebid requirements (prebid#974)
  Use stable version of Chrome (prebid#984)
  Revert to running browser tests in Travis (prebid#983)
  Fix issue with appnexusAst sending `user` object in the wrong place. (prebid#980)
  Integrate Browserstack tests into Travis CI build (prebid#839)
  Add StickyAdsTV Bidder adapter (prebid#916)
  Stronger xdomain checks (prebid#971)
  added matomy as an alias for appnexus (prebid#850)
  Added 152Media Appnexus Alias (prebid#952)
  OpenX Adapter: Handles fallback ads correctly as a no fill (prebid#39) (prebid#963)
  Use package dependencies for ES6 Array shims (prebid#962)
  Make x-domain safe frame example work out of the box (prebid#955)
  added usersync for adkernel adapter (prebid#951)
  Rubicon adapter: add a floor variable (prebid#964)
  Added Lifestreet adapter. (prebid#965)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…14.0 to master

* commit 'c008f3f531ae3409f4a16bf03470d84e82aead0e': (35 commits)
  Add adapters in aolPartnersIds.json.
  Add changelog entry.
  Prebid 0.19.0 Release
  check truthiness of adUnitCode (prebid#990)
  fixed exception when refreshing individual Ad Units and bidder responds slowly (prebid#989)
  Stub pixel drop to prevent network request in test (prebid#988)
  Updating Komoona adapter to support future Prebid requirements (prebid#974)
  Use stable version of Chrome (prebid#984)
  Revert to running browser tests in Travis (prebid#983)
  Fix issue with appnexusAst sending `user` object in the wrong place. (prebid#980)
  Integrate Browserstack tests into Travis CI build (prebid#839)
  Add StickyAdsTV Bidder adapter (prebid#916)
  Stronger xdomain checks (prebid#971)
  added matomy as an alias for appnexus (prebid#850)
  Added 152Media Appnexus Alias (prebid#952)
  OpenX Adapter: Handles fallback ads correctly as a no fill (prebid#39) (prebid#963)
  Use package dependencies for ES6 Array shims (prebid#962)
  Make x-domain safe frame example work out of the box (prebid#955)
  added usersync for adkernel adapter (prebid#951)
  Rubicon adapter: add a floor variable (prebid#964)
  ...
@nanek
Copy link
Contributor

nanek commented Apr 19, 2017

@outoftime Do you recall what version of IE? I'm testing our site in IE 9 and it works fine in Prebid 0.18 without this PR. Could this have been something else in your app?

@mkendall07
Copy link
Member

Good point, the screenshot references another script file and we test extensively in IE.

@nanek nanek mentioned this pull request Apr 20, 2017
1 task
@outoftime
Copy link
Contributor Author

The problem was observed in IE 11. I can tell you that the change in this PR, in isolation, fixed the error. You’re correct that the error was being thrown from our main application bundle, so it’s possible that the original polyfill differed subtly from real browser behavior in terms of the property descriptor for the polyfilled methods, which caused another library in our bundle to break.

Anyway, at this point we bundle Prebid into a JS bundle that we control, so if you roll this back we can still work around the problem by importing an appropriate polyfill before importing Prebid.

@mkendall07
Copy link
Member

We are gonna to roll back this change, however, I'm not sure it's causing the issue you think it is. I just ran this code in IE 11: https://jsfiddle.net/tn3d87zy/ and don't see any exception.

Also - note that the same code is used in the polyfill includes as it was with the manual includes we had (it does a check on !Array.prototype.find and replaces if not found).

@outoftime
Copy link
Contributor Author

outoftime commented Apr 20, 2017

@mkendall07 yeah it’s hard to say at this point, although maybe I can get more info on the error once this is rolled back. I think it’s probably more subtle than a transparent error in redefining find, but again, this PR alone made the difference between broken and not-broken for us.

@adamjung adamjung deleted the array-shims branch November 25, 2019 15:21
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

Successfully merging this pull request may close these issues.

4 participants