-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
now works browserify, considering publishing to npm #76
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
Conversation
The patch looks good. But I don't really get the rationale. SockJS-client only works in browser context, right? Can you explain would I like to do 'require' on it? |
Module systems like browserify let you do |
Merged as 45417a1 to dev branch. Thanks! |
I guess I should push sockjs-client to npm now, right? |
I am trying to bundle this package with browserify. Looks like this patch never made it to master, or did it? browserify is a great package manager and also allows to share modules between client and server. we find it very useful. Also looked into bower with no luck. |
Did this ever happen? This version of |
Any chance this will be resolved? I'd like to use Sockjs with browserify and this is a blocker. |
+1 |
+1 https://www.npmjs.org/package/sockjs-client appears to be a Node client, not a browser client |
why did this never make it in? |
I will get this published soonish. I think there is some confusion around this and the node SockJS client. Ideally there wouldn't be two clients, but that might take more time. Ideas on how to clarify until then? |
maybe in the name? |
Why is there a difference? |
I'm guessing because on node it simply uses node's http library, but in the browser the transports are WebSocket, xhr, iframe, all the other fallbacks etc. And they don't want to ship node code to browsers or browser code to node. Just guessing |
IMO sockjs should take the same approach engine.io takes with this problem - one library but skip the upgrade chain and go straight to websockets on node |
yes, because building the exact same library makes sense right ;-)? |
The usage of the |
1.0 will have the same client for node and browsers published to npm as |
Heads up that |
These patches detect the presence of
module.exports
and assign accordingly. I saw there was already a shim for AMD define() so I put this patch right next to it.I also added a check to see if
this === window
in the constructor so that thenew
is optional in front ofSockJS
. Plus I added avar
in front of theSockJS =
so that the SockJS reference won't leak out when the whole module is wrapped in a closure, like browserify does. In the main window context doingvar
assigns ontowindow
anyways so this change shouldn't have any impact.I put some things in the .npmignore that would likely be troublesome in publishing to npm and added a "main" field, so this package should be suitable for an
npm publish
! Just adjust the version field.When this package hits npm in browserify people will just be able to
require('sockjs-client')
which is much handier