search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

Attempt to test on IE 7 & 8 #301

Closed ScottFreeCode closed 6 years ago

ScottFreeCode commented 7 years ago

This may not actually run the browser tests unless you pull it down locally or recreate it yourself in a branch on the original project rather than my fork -- I believe Travis + SauceLabs does not test PRs unless you've set it up to use JWT, for instance. I also don't know for certain that the test usage matches Mocha's Browserify-based usage.

That being said, if older versions of IE are meant to be supported we may as well try to see all the possible errors in one go instead of repeatedly running Mocha's tests and finding the next compatibility problem; hopefully this is a move in that direction.

(See #293 for discussion so far, and the latest example is: https://travis-ci.org/mochajs/mocha/jobs/243848563#L2346 https://github.com/nodejs/readable-stream/blob/master/lib/_stream_readable.js#L189 )

mcollina commented 7 years ago

I think we have reached a point where we cannot support those browsers anymore. That Object.defineProperty is not avoidable (like the previous ones). Il giorno ven 23 giu 2017 alle 05:11 Scott Santucci < notifications@github.com> ha scritto:

This may not actually run the browser tests unless you pull it down locally or recreate it yourself in a branch on the original project rather than my fork -- I believe Travis + SauceLabs does not test PRs unless you've set it up to use JWT, for instance. I also don't know for certain that the test usage matches Mocha's Browserify-based usage.

That being said, if older versions of IE are meant to be supported we may as well try to see all the possible errors in one go instead of repeatedly running Mocha's tests and finding the next compatibility problem; hopefully this is a move in that direction.

(See #293 https://github.com/nodejs/readable-stream/issues/293 for discussion so far, and the latest example is: https://travis-ci.org/mochajs/mocha/jobs/243848563#L2346

https://github.com/nodejs/readable-stream/blob/master/lib/_stream_readable.js#L189 )

You can view, comment on, or merge this pull request online at:

https://github.com/nodejs/readable-stream/pull/301 Commit Summary

  • Attempt to test on IE 7 & 8

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/readable-stream/pull/301, or mute the thread https://github.com/notifications/unsubscribe-auth/AADL40YVjQZdml5wZMRPGtcEtaEfo5Y1ks5sGyzQgaJpZM4ODFbE .

mcollina commented 7 years ago

The only way this might be possie is if we want to selectively remove that property if it is not available, and let IE 7 & 8 run without the destroyed property. If that is even possible.

calvinmetcalf commented 7 years ago

I belive we stopped testing because other stuff in the test setup didn't support ie 7 and 8

On Fri, Jun 23, 2017, 2:23 AM Matteo Collina notifications@github.com wrote:

The only way this might be possie is if we want to selectively remove that property if it is not available, and let IE 7 & 8 run without the destroyed property. If that is even possible.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/readable-stream/pull/301#issuecomment-310582791, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4nybnK-j8ZYVJFVXtvf2tbzoH5inTks5sG1nogaJpZM4ODFbE .

calvinmetcalf commented 7 years ago

For mochas purposes just including a shim might fix

On Fri, Jun 23, 2017, 6:47 AM Calvin Metcalf calvin.metcalf@gmail.com wrote:

I belive we stopped testing because other stuff in the test setup didn't support ie 7 and 8

On Fri, Jun 23, 2017, 2:23 AM Matteo Collina notifications@github.com wrote:

The only way this might be possie is if we want to selectively remove that property if it is not available, and let IE 7 & 8 run without the destroyed property. If that is even possible.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/readable-stream/pull/301#issuecomment-310582791, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4nybnK-j8ZYVJFVXtvf2tbzoH5inTks5sG1nogaJpZM4ODFbE .

ScottFreeCode commented 7 years ago

The only way this might be possie is if we want to selectively remove that property if it is not available, and let IE 7 & 8 run without the destroyed property. If that is even possible.

For mochas purposes just including a shim might fix

I think I'll give this a shot via shimming and see where it leads; I was hesitant to fiddle too much with defineProperty myself because, as far as I'm aware, it can't actually recreate the effects of getters and setters so a shim would have to either make them plain properties or not include them at all, but if that's the most reasonable thing to try, then I'm game! Thanks for the feedback.

Also thanks for getting Phantom fixed, in any case, since that means if we do end up having to drop older Internet Explorer we at least have the option of sticking with Phantom if we need to for some reason.

mcollina commented 7 years ago

That feature is new, and it has been introduced in v2.3.0 (and Node 8). So, you are very likely not to use it. A solution on our side is to provide a fake (doing nothing) defineProperty. This means old browsers won't have the destroyed property, but it is a new addition anyway.

ScottFreeCode commented 7 years ago

I just figured out that this is unlikely to be safely shimmable on Mocha's end. Since Object.defineProperty is global and readable-stream gets bundled into the browser version of Mocha (used not just by Mocha's own tests but by downstream users of Mocha), to polyfill it for Mocha as far as I can tell we'd have to effectively polyfill it for all browser tests using Mocha. If so, that would cause other projects' Mocha-based tests to pass in situations like this where defineProperty creates something that doesn't end up being used, even though the same code will fail in production on older browsers unless the application provides its own polyfill for Object.defineProperty.

mcollina commented 6 years ago

Closing as we will not support IE 7 and IE 8 in readable-stream@3 (https://github.com/nodejs/readable-stream/pull/344).