Replace socket.io with SockJS as discussed in #229 #302
Conversation
This removes the node-gyp dependency :). Fixes webpack#229, webpack#276, fixes webpack#195, fixes webpack#267, fixes webpack#258 and fixes webpack#242
|
|
||
| // Try to reconnect. | ||
| sock = null; | ||
| recInterval = setInterval(function () { |
There was a problem hiding this comment.
Since you are clearing the interval immediately on execution, why not just:
recTimeout = setTimeout(newConnection, 2000, handlers);
Since newConnection is only called once, the clearInterval is not needed. Same for the other thing in live.js.
There was a problem hiding this comment.
Good point, this is fixed now.
|
+1 for replacing Socket.io with something more lightweight :) |
|
I have the same problem. How to replace socket.io with SockJS? |
|
@aromot, you'll have to wait until this PR is merged and released. |
|
@SpaceK33z This would be a welcome change! Is there some sort of built version somewhere, or is using https://github.com/CodeYellowBV/webpack-dev-server/tree/sockjs as the dependency sufficient? What versions of Node are supported? |
| @@ -146,7 +151,7 @@ function Server(compiler, options) { | |||
| } | |||
| proxy.web(req, res, proxyOptions, function(err){ | |||
| var msg = "cannot proxy to " + proxyOptions.target + " (" + err.message + ")"; | |||
| this.io.sockets.emit("proxy-error", [msg]); | |||
| this.sockWrite("proxy-error", [msg]); | |||
There was a problem hiding this comment.
Should this also be guarded with a verification that this.sock exists? I get a crash when this runs because this.sock is undefined in the this.sockWrite call. Perhaps that guard should simply be put inside the sockWrite call?
There was a problem hiding this comment.
There is a guard inside of sockWrite
There was a problem hiding this comment.
Yes, it was added following my remark. :) See CodeYellowBV@a5ddf82
|
@mathieumg Looking at the You could test it manually by adding my github repo as a dependency, I can't find the exact node version requirements for the |
|
Yeah I had figured that in the meantime! :) I did and I got a crash, I pointed where in the review, above! |
|
@mathieumg, could you try again? I have probably fixed your issue. Weirdly enough I never had this issue. And thanks for testing, I appreciate it :). |
|
@SpaceK33z Everything works perfectly with the latest fix! :) |
|
What about server sent events, should be simpler and a small dependency for one way notifications. https://github.com/glenjamin/webpack-hot-middleware uses SSE and the code looks really simple. SSE should be easy to polyfill anywhere that has http. |
|
@sokra Could we hear your opinion on this PR? It would solve the issue with dependency on the github package from socket.io and would be more lightweight. |
| if (this.sock) { | ||
| this.sock.write(JSON.stringify({type: type, data: data})); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you move this function into the prototype of the class?
There was a problem hiding this comment.
Good point, fixed that now.
| this.io.sockets.on("connection", function(socket) { | ||
| if(this.hot) socket.emit("hot"); | ||
| sockServer.on("connection", function(conn) { | ||
| this.sock = conn; |
There was a problem hiding this comment.
It should be possible to connect multiple devices to the same dev-server.
There was a problem hiding this comment.
Ah good point, this could also be the reason that a re-connect doesn't emit an ok.
There was a problem hiding this comment.
Wow, that was a pretty big oversight of me. Pretty ashamed I didn't notice that myself. Anyway, it should be fixed now.
|
hi @SpaceK33z also checked the fix and the webpack dev server is up and running on windows. any idea when the fixed will be merged and published ? 10x |
|
Looks fine now... I'll test it and eventually merge it. Please ping me in a week if I forget it... |
|
+1 wait for it. |
|
Important update - in the latest version of Artifactory there is support for this kind of problems (direct http or github resources) via the NPM/Bower |
Replace socket.io with SockJS as discussed in #229
|
Thanks 👍 pretty good change, finally we get rid of the native dependency... |
|
Great job guys! Thank you! |
|
Awesome! 🎉 |
|
thanks :-) |
|
\o/ |
|
thanks |
|
How does this fix #276? Theres still a ton of client side logging and I dont see how to disable it.
|
|
Yeah that's because you have enabled it in your If you want debugging logs again: |
|
That was it, thanks! |
ghost
mentioned this pull request
| var currentHash = ""; | ||
|
|
||
| var newConnection = function(handlers) { | ||
| sock = new SockJS('/sockjs-node'); |
There was a problem hiding this comment.
Just wanted to flag that this breaks certain usage that relied on the previous /socket.io path. They now have to be /sockjs-node. Should this have been a major version bump, according to semver?
|
i still do not know how to solve my trouble, it means that i should get the latest version of webpack-dev-server? or? |
The socket.io dependency causes node-gyp to be used. This results in a lot of issues with Windows and Linux installs. SockJS is much, much lighter and is node-only. This also results in a 5mb package size decrease :).
This is a relatively big change, so it would be best if some people test this in their project.
Fixes #229, #276, fixes #195, fixes #267, fixes #258 and fixes #242