-
Notifications
You must be signed in to change notification settings - Fork 230
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
Attempt to test on IE 7 & 8 #301
Conversation
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 <#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:
#301
Commit Summary
- Attempt to test on IE 7 & 8
File Changes
- *M* .travis.yml
<https://github.com/nodejs/readable-stream/pull/301/files#diff-0> (2)
Patch Links:
- https://github.com/nodejs/readable-stream/pull/301.patch
- https://github.com/nodejs/readable-stream/pull/301.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#301>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADL40YVjQZdml5wZMRPGtcEtaEfo5Y1ks5sGyzQgaJpZM4ODFbE>
.
|
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. |
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 ***@***.***> 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
<#301 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABE4nybnK-j8ZYVJFVXtvf2tbzoH5inTks5sG1nogaJpZM4ODFbE>
.
|
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 ***@***.***>
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
> <#301 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABE4nybnK-j8ZYVJFVXtvf2tbzoH5inTks5sG1nogaJpZM4ODFbE>
> .
>
|
I think I'll give this a shot via shimming and see where it leads; I was hesitant to fiddle too much with 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. |
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. |
I just figured out that this is unlikely to be safely shimmable on Mocha's end. Since |
Closing as we will not support IE 7 and IE 8 in readable-stream@3 (#344). |
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
)