Temasys / AdapterJS

AdapterJS Javascript Polyfill and Tools for WebRTC - Skylink WebRTC
http://skylink.io/web
Other
428 stars 100 forks source link

Add polyfill for Array.prototype.forEach. #287

Closed oooookk7 closed 6 years ago

oooookk7 commented 6 years ago

Simple PR. Noticed that Array.prototype.forEach is used in line 178, line 1165 and line 1210. But according the MDN docs for it IE 8 does not support that function natively although we don't recommend that browser version. This is just to polyfill it when it's missing to prevent errors.

kekkokk commented 6 years ago

tried today the master branch but without polyfill I cannot access to navigator.mediaDevices.desiredfunction and other functions of the native adapter. This way I'll have to change all my code to support AdapterJS calls instead of native ones. why don't just include a promise polyfill?

oooookk7 commented 6 years ago

@johache understand. If I recall though, it works fine in my Windows XP IE 8 before (I might have resolved some polyfills to workaround). Perhaps instead of using forEach(), we use while or for loops instead so we don't limit completely the support?

johache commented 6 years ago

As a default, I'm against making changes to support browsers that we don't advertise support for... Just because it happened to work at some point (by luck) doesn't mean we should go and alter native prototypes to maintain support in the present.

@ncurrier to arbitrate here

oooookk7 commented 6 years ago

Generally, I agree that we should not advertise support for these browsers, but I do not see why it should break the code execution for a version that is close the minimal support like IE 8 (it's not IE 4) which we support 9 and above. It might be ideal that it should not break the code execution and users should display in the UI that "Please use IE 9 since IE 8 is not supported" with detection based on AdapterJS.webrtcMinimumVersion. My point is to just use for loop instead of forEach().

We can close this PR for something else where we use for loops instead of the prototype polyfill.

oooookk7 commented 6 years ago

Closing because if users want to integrate themselves with no guarantees, they can append the polyfill themselves before the <script src="adapter.debug.js"></script>.