SocketCluster / socketcluster-client

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

`global.WebSocket` throws in some eviroments #45

Closed zalmoxisus closed 8 years ago

zalmoxisus commented 8 years ago

As you see from https://github.com/zalmoxisus/remote-redux-devtools/issues/6, sometimes either babel or react-native packager adds 'use strict'; to the dependencies, and this makes global to be undefined.

Maybe in browser.js instead of var WebSocket = global.WebSocket || global.MozWebSocket, we could use var WebSocket = this.WebSocket || global.WebSocket || global.MozWebSocket as @mlabrum suggested?

jondubois commented 8 years ago

@zalmoxisus Does this affect the latest version of socketcluster-client? I remember fixing something like this a while ago: https://github.com/SocketCluster/socketcluster-client/issues/40

The issue was in the ws module - In response to this, we rolled back to using our own fork (sc-ws) of the ws module. See https://github.com/SocketCluster/socketcluster-client/commit/c9cb35172b493159611346f4165ad3a582d21c45

I thought that fixed the issue?

zalmoxisus commented 8 years ago

Yes, we use the latest version. The error comes from node_modules/sc-ws/lib/browser.js:

/**
 * Module dependencies.
 */

var global = (function() { return this; })();

/**
 * WebSocket constructor.
 */

var WebSocket = global.WebSocket || global.MozWebSocket;

To reproduce it, just add 'use strict'; at the beginning of that file (as babel or react-packager does). So then var global = (function() { return this; })(); will be undefined and in the next line we are trying to read the property of undefined.

jondubois commented 8 years ago

@zalmoxisus Ok this should be fixed in socketcluster-client v4.3.10 - Make sure it also updates to the latest sc-ws module v1.0.2 https://www.npmjs.com/package/sc-ws

zalmoxisus commented 8 years ago

@jondubois, thanks a lot for the quick fix! Tested, it solves the problem with Babel. Don't have the possibility to check with React Native now, but, looking through the sources, it should expose window object.

zalmoxisus commented 8 years ago

It seems there are more issues in other modules to get socketcluster-client work in strict mode.

With https://github.com/facebook/react-native/pull/5796 now React Native adds strict mode by default.

zalmoxisus commented 8 years ago

Looking into it, the strict mode doesn't allow arguments.callee used in sc-errors(Why was arguments.callee removed from ES5 strict mode?).

But it should be another problem that calls the error function. I'll try to investigate it later and come with a solution.

jondubois commented 8 years ago

@zalmoxisus Ok this should be fixed v4.3.11, you need to make sure to update the sc-errors dependency.

I'm not sure why it's throwing the TimeoutError... This can happen if you did a socket.emit('myEvent', data, callback); on the client (expecting a callback) and then on the server you forget to invoke the respond function socket.on('myEvent', function (data, respond) { ... You need to invoke the respond() method here or else the client-side callback will timeout ...}).

You don't have to call respond() on the server if your client emit doesn't have a callback though...

zalmoxisus commented 8 years ago

@jondubois, thanks for the fix and for the detailed explanation! Actually, I cannot reproduce that error in a non React Native environment. The client has a callback and the server responds to it. It could be related to https://github.com/zalmoxisus/remote-redux-devtools/issues/4. Will investigate further.

zalmoxisus commented 8 years ago

It works. Thanks a lot, @jondubois!