Skip to content

Commit

Permalink
- W3CWebSocket: Use just one or two arguments (fixes #173)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ibc
ibc committed Dec 2, 2014
1 parent b57669c commit 12a32d9
Showing 1 changed file with 19 additions and 3 deletions.
22 changes: 19 additions & 3 deletions lib/browser.js
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,32 @@
var _global = (function() { return this; })();
var nativeWebSocket = _global.WebSocket || _global.MozWebSocket;


/**
* W3CWebSocket constructor.
* Expose a W3C WebSocket class with just one or two arguments.
*/
var W3CWebSocket = _global.WebSocket || _global.MozWebSocket;
function W3CWebSocket(uri, protocols) {
var instance;

if (protocols) {
instance = new nativeWebSocket(uri, protocols);
}
else {
instance = new nativeWebSocket(uri);
}

return instance;
}

if (nativeWebSocket) {
W3CWebSocket.prototype = nativeWebSocket.prototype;
}


/**
* Module exports.
*/
module.exports = {
'w3cwebsocket' : W3CWebSocket ? W3CWebSocket : null,
'w3cwebsocket' : nativeWebSocket ? W3CWebSocket : null,
'version' : require('./version')
};

20 comments on commit 12a32d9

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why can't we just directly return nativeWebSocket here?

module.exports = {
    'w3cwebsocket' : nativeWebSocket,
    'version'      : require('./version')
};

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when using the W3CWebSocket function as a constructor, you can't return instance to change the object that is constructed. I really don't see the purpose of this?

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see, it's to be able to accept a third non-standard options object that will be ignored in the browser.

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, if used in the manner below, I can't imagine how this could possibly work:

var W3CWebSocket = require('websocket').w3cwebsocket;

var connection = new W3CWebSocket('ws://localhost:8080/', 'echo');
connection.onmessage = function() { /* ... */ };

By the time we attach the onmessage handler, it theoretically wouldn't attach to the "real" native WebSocket implementation, it would attach to an instance of our fake class-factory wrapper because you can't implement the class-factory pattern using constructors in JavaScript. When you return another object from a constructor in JavaScript, that's just ignored and it returns this anyway.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure in Chrome/Firefox (I should have tested this more) but in Android WebView (using Cordova) the WebSocket constructor fails if it's given with more than two arguments. So we need to "filter" them. For that I added a "faked" constructor (w3cwebsoket.nativeWebSocket) which takes the first one or two arguments and pass them to the real WebSocket class (which in this case is the browser's native WebSocket implementation).

when using the W3CWebSocket function as a constructor, you can't return instance to change the object that is constructed.

The instance is not changed but just returned.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the time we attach the onmessage handler, it theoretically wouldn't attach to the "real" native WebSocket implementation, it would attach to an instance of our fake class-factory wrapper because you can't implement the class-factory pattern using constructors in JavaScript. When you return another object from a constructor in JavaScript, that's just ignored and it returns this anyway.

The trick here is in the last line:

W3CWebSocket.prototype = nativeWebSocket.prototype;

Here the full prototype of the native WebSocket class is set into the W3CWebSocket class-factory wrapper.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is tested:

$ node

> var W=require("./").w3cwebsocket

> w=new W('ws://ws1.versatica.com:10080', 'sip', 'http://tryit.jssip.net');   w.onopen=function() { console.log("OPEN"); }

> OPEN

;)

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it turns out I'm wrong! Sweet! 😁

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being honest I don't like this "workaround" but I cannot figure out a better one. And given that ws uses the same for long time (and nobody has reported that it fails) I accept it as a "valid" solution.

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was pretty certain that you couldn't override the object that was returned from a constructor with another object. The fact that you can opens up a lot of possibilities. It's nice to be proven wrong once in a while! I get to learn! 😀

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm also annoyed by the fact that a constructor can return something different than this... But you know: this is JavaScript, anything is possible.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wrong. The wrapper constructor does not return instance. I'll prove it in 2 minutes.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was right. Demonstration:

Wrong (since C1Wrapper does not return c1):

function C1(num) {
    console.log('C1()');

    this.num = num;

    // Call a "private" method of C1.
    privateMethod.call(this);
}

C1.prototype.hello = function() {
    console.log('C1#hello() | my num is %d', this.num);
}

function privateMethod() {
    console.log('C1#privateMethod() | my num is %d', this.num);
}



function C1Wrapper(num) {
    console.log('C1Wrapper()');

    var c1 = new C1(num);
}

C1Wrapper.prototype = C1.prototype;



// Test it:

var c = new C1Wrapper(1234);

console.log('c is:', c);

c.hello();

Output:

C1Wrapper()
C1()
C1#privateMethod() | my num is 1234
c is: {}
C1#hello() | my num is NaN    // <----- ERROR

Correct

Just add return c1 in the C1Wrapper constructor.

Output:

C1Wrapper()
C1()
C1#privateMethod() | my num is 1234
c is: { num: 1234 }
C1#hello() | my num is 1234

"Love" JavaScript...

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://codepen.io/theturtle32/pen/xbwMKX?editors=101

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proved now :)

@theturtle32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I released v1.0.14 btw.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For funny, change this:

function AnotherClass() {
  var instance = new TestClass();
  return "what will happen now?";
}

Don't ask me about why it happens what it happens...

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! time to update libraries!

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I've realized that the line W3CWebSocket.prototype = nativeWebSocket.prototype; is not needed at all (it works without it). It makes sense given that the returned instance of the wrapper-class is in fact an instance of the original class.

@ibc
Copy link
Collaborator Author

@ibc ibc commented on 12a32d9 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More about returning stuff in a constructor: http://www.bennadel.com/blog/2522-providing-a-return-value-in-a-javascript-constructor.htm

Please sign in to comment.