throw a exception when be imported into Browser (#1344)

[huan]

#1344

[lpinca]

Add browser.js

[lpinca]

Please keep the limit of 80 chars per line. Also, I don't want to endorse any particular wrapper, we are already doing it in the README and there is no need to point to that particular issue, there a few already which are all duplicates.

I'd just use a very simple message like "ws does not work in the browser. Browser clients must use the native WebSocket object". I think this is sufficient to point the user into the right direction.

[huan]

@lpinca Changed as you request.

[lpinca]

cc: @3rd-Eden

[3rd-Eden]

I'm fine with this but it might be a major release as there is a chance of breaking people's code if they do a weird fallback like:

var WS = require('ws') || WebSocket;

[huan]

@3rd-Eden Thanks for accepting this.

I feel this will not breaking anything because according to the current code base, the return value of require('ws') will neither be undefined nor null, so the || WebSocket part in the fallback you mentioned will never get chance to be executed.

[lpinca]

@zixia I think @3rd-Eden wanted to write:

var WS = global.WebSocket || require('ws');

I think this will throw now if the code is bundled for Node.js?

[huan]

@lpinca I believe it will not throw in the Node.js environment.

Because in Node.js global.WebSocket will be undefined, then WS will be set as require('ws'), which should be right.

[lpinca]

Yes but isn't the browser shim used in that case?

[lpinca]

It throws

$ cat index.js 
const WS = global.WebSocket || require('ws');

const ws = new WS('ws://echo.websocket.org');
ws.onopen = function () {
  console.log('It works');
};
$ browserify -u bufferutil -u utf-8-validate --no-builtins --insert-global-vars="global" index.js > bundle.js
$ node bundle.js

[lpinca]

The --no-browser-field option can be used to make it work again but I think it's still a breaking change.

[huan]

Yes, you are right.

I did not consider the browserify case.

[shogowada]

I've read #1344, but I am personally not sure why it needs to throw when it can fallback. It looks like we are waiting for @3rd-Eden 's input?

Maybe it's already discussed at other places, but should this fallback to browser's WebSocket, while announcing that WebSocket.Server doesn't work on browser?

Although, I see that ws's API isn't 100% compatible with browser's WebSocket, so I understand if we don't want to mislead people that it should work seamlessly on both browser and Node.js 100% of the time.

Given that, I think falling back to browser's WebSocket would make many projects' dependency graph a bit cleaner and more straightforward when they need to implement a platform agnostic web socket client, and I think it would make ws package more attractive.

[lpinca]

@shogowada this has been discussed ad nauseam, we don't want to maintain a browser shim because there are so many differences/edge cases that makes it a pain.

Refs: e54d45f

[shogowada]

Thanks for the additional context.

In that case, instead of throwing (that appears to break some), would a warning work?

Something like this:

module.exports = function () {
  // Almost a copy & paste from the README + link to the ref you gave me for future people like me🙂
  console.warn('ws module does not work in the browser. Browser clients must use the native WebSocket object. To make the same code work seamlessly on Node.js and the browser, you can use one of the many wrappers available on npm, like isomorphic-ws. For more context, see https://github.com/websockets/ws/commit/e54d45fbab3b19c2940e9057ce1e7b8f105873e0.')
}

[lpinca]

I don't think it helps, it still breaks existing code. Throwing an error is the right thing to do imho.

[lpinca]

Finally merged. Thank you.

[huan]

Glad to see that, cheers!