SocketCluster / socketcluster-client

JavaScript client for SocketCluster
MIT License
293 stars 91 forks source link

Version 4.x is not compatible with Webpack #40

Closed zalmoxisus closed 8 years ago

zalmoxisus commented 8 years ago

Due to the issue https://github.com/websockets/ws/issues/662, replacing sc-ws with ws in 4.x brokes Webpack compatibility.

zalmoxisus commented 8 years ago

Downgrading ws' to0.8.1` helps.

mattkrick commented 8 years ago

^^ can confirm. downgrading the socketcluster-client dep to 0.8.1 fixes this problem. for the googlers, sticking to v4.1.2 solves this

jondubois commented 8 years ago

@mattkrick Is this an issue with the ws module itself or how SC uses it? Does the error happen if you try to bundle the ws module on its own? Could it be this line that's causing the problem: https://github.com/SocketCluster/socketcluster-client/blob/master/lib/sctransport.js#L6 ? Does Webpack support the global variable?

mattkrick commented 8 years ago

Is this an issue with the ws module itself or how SC uses it? the ws module itself

Does the error happen if you try to bundle the ws module on its own? Yep, it would

Could it be this line that's causing the problem: https://github.com/SocketCluster/socketcluster-client/blob/master/lib/sctransport.js#L6 ? Nope, it's this one: https://github.com/SocketCluster/socketcluster-client/blob/master/lib/sctransport.js#L1

Does Webpack support the global variable? Yep, but that's unrelated

The problem is that webpack sees socketcluster-client & bundles it for use in the browser. Then, when it comes along & sees a require('fs') it doesn't know what to do with that, since fs isn't available on the browser. For certain things, like the require('tls') it can mock the package with enough functionality to not throw an error, but ya can't mock fs. The problem actually has nothing to do with socketCluster-client, it's because you have a dependency to something that expects a server environment.

jondubois commented 8 years ago

I guess it's because of a change in ws - Apparently it was a problem for a lot of people. See comments here: https://github.com/websockets/ws/commit/e54d45fbab3b19c2940e9057ce1e7b8f105873e0

mattkrick commented 8 years ago

yeah, that's no good. Couple work arounds:

mattkrick commented 8 years ago

any details on why you use it as a fallback? Is it just for old browsers? Going through the ws package, it's clear they want it to be node-only & what socket.io did to fix it seems... not right.

jondubois commented 8 years ago

No, it shouldn't be bundled in the browser. Ideally browserify and webpack should just ignore it. But they don't.

mattkrick commented 8 years ago

if it shouldn't be bundled in the browser why is it in socketcluster-client? (i'm on gitter if you wanna continue there)

jondubois commented 8 years ago

@zalmoxisus @mattkrick Ok, it should now be fixed in v4.1.4.

sjmueller commented 8 years ago

Unfortunately this is still broken in react-native, even as of v4.1.4.

jondubois commented 8 years ago

@sjmueller Does your react-native setup use webpack? What error do you get?

sjmueller commented 8 years ago

@jondubois it uses babel 6. I think the transpilation is pretty aggressive, and that maybe it's building a dependency graph whenever it sees a require statement. So even though require('ws') is hiding behind the browser websocket checks, it's still walking through ws and giving dependency errors:

Error: UnableToResolveError: Unable to resolve module url from /node_modules/
socketcluster-client/node_modules/ws/lib/WebSocket.js:
Invalid directory /Users/node_modules/url
jondubois commented 8 years ago

So the current best solution to the ws browser-build problem works with browserify and webpack but not babel.

I forked ws to sc-ws temporarily (and reverted some breaking changes from the ws module) until someone comes up with a solution which works with browserify, webpack and babel... Or until babel catches up and starts working with the solution proposed here: https://github.com/websockets/ws/commit/e54d45fbab3b19c2940e9057ce1e7b8f105873e0#commitcomment-15385077

It should all be fixed in socketcluster-client 4.1.5.

mattkrick commented 8 years ago

@sjmueller can you give some more details on this? i don't understand how babel 6 could be causing this, all it does is transpile, and a require statement in ES5 is gonna be the same as it is in ES6+ (ignoring the deafult behavior change in babel 6). If you exclude socketcluster-client from your babel build does that fix it?

zalmoxisus commented 8 years ago

Tested 4.1.6 on Webpack and React Native and there aren't any problems anymore. And since there are no plans to restore browser code in ws, I'm closing the issue.

sukeshthakare commented 7 years ago

I also get same error on Windows 10..

8:40:50 PM: (node:6424) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2865): UnableToResolveError: Unable to resolve module C:\Users\Sukeshu\Desktop\abc\SocialApp\node_modules\socketcluster-client\lib\sctransport.j\lib\ws-browser.js from C:\Users\Sukeshu\Desktop\abc\SocialApp\node_modules\socketcluster-client\lib\sctransport.js: Directory C:\Users\Sukeshu\Desktop\abc\SocialApp\node_modules\socketcluster-client\lib\sctransport.j\lib\ws-browser.js doesn't exist