-
Notifications
You must be signed in to change notification settings - Fork 80
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
Browserify-compatible #56
base: master
Are you sure you want to change the base?
Conversation
+1 |
2 similar comments
+1 |
👍 |
|
But this is a PR with those changes. Only someone with |
Doh, I clearly didn't get enough sleep last night... |
@flippyhead I can only speak for myself, but the issue here is that adding browser support massively increases the test/validation/support surface of this package. It would be trivial to merge this PR and push a new version of the package to npm. But highly non-trivial to ensure that it works correctly on every browser implementing websockets. And what happens with older browsers? I guess what I'm saying is that this feels more experimental than production, and tons of production things depend on this package, So absent someone who is willing to make the effort to (re)write all of the test cases for the browser environment, and then actually test across a bunch of different browsers, and also commit to fielding the inevitable questions and fix the issues that arise... My 2¢ on why nobody has jumped on it (myself included). |
var WebSocket; | ||
|
||
var isBrowserified = (typeof window !== 'undefined'); | ||
if (isBrowserified) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the presence of window.Websocket
should be the actual test here. And code should be added to detect when it is being run in a non-Websocket implementing browser and do something more helpful that throwing during package load on the absence of window.Websocket
(which is presumably what happens now in that case)
Thank your for this, this stuff is awesome.
It can be used in the browser too! Browserify can be used to get it in the browser, but a minor tweak is needed.
faye-websocket
is not browserify-compatible and not needed since most modern browsers have WebSocket abilities themselves.