search

max-mapper / websocket-stream

websockets with the node stream API
BSD 2-Clause "Simplified" License
668 stars 114 forks source link

check for global.MozWebSocket in native browser case #86

Closed ralphtheninja closed 8 years ago

ralphtheninja commented 8 years ago

If ws-fallback.js is used for the browserified code, shouldn't the isNative check also check for that?

deanlandolt commented 8 years ago

At first glance this seems about right, though IIRC the issue the isNative test is used to guard for doesn't surface in the vendorized window.MozWebSocket but only in FF's implementation of window.WebSocket -- which (annoyingly) adheres to the letter of the spec. Perhaps we should change this to bool to isStandard, or throwsPointlessExceptionOnNonstandardArguments...

In that light, this patch shouldn't be necessary. Was there some behavior you're seeing with window.MozWebSocket which suggests otherwise? If so, what version of FF?

ralphtheninja commented 8 years ago

In that case I propose we rename the variable to something else than isNative which sounds more generic.

deanlandolt commented 8 years ago

:+1:

It tripped me up too. Any ideas? I assume throwsPointlessExceptionOnNonstandardArguments is a bit too...longwinded :D

ralphtheninja commented 8 years ago

Or maybe we can scrap this patch and remove MozWebSocket from the fallback? See https://github.com/tornadoweb/tornado/pull/752

deanlandolt commented 8 years ago

That'd be fine by me -- would add a bit of clarity to the code, though I'm not sure it buys us much else, and could affect an (admittedly small) number of users. Anyone else have an opinion on this?

mcollina commented 8 years ago

I think we can remove MozWebSocket from the fallback.

ralphtheninja commented 8 years ago

I think we can remove MozWebSocket from the fallback.

:+1: