theturtle32 / WebSocket-Node

A WebSocket Implementation for Node.JS (Draft -08 through the final RFC 6455)
Apache License 2.0
3.75k stars 604 forks source link

Why .type == 'utf8'? #132

Closed ghost closed 9 years ago

ghost commented 10 years ago

Why introducing new objects and properties and stuff where it could made so much more elegant? Is there a reason I don't see?

Instead of creating new object encapsulating the native JS/node.js type for each message

        if (message.type === 'utf8') {
            console.log('Received Message: ' + message.utf8Data);
            connection.sendUTF(message.utf8Data);
        }
        else if (message.type === 'binary') {
            console.log('Received Binary Message of ' + message.binaryData.length + ' bytes');
            connection.sendBytes(message.binaryData);
        }

Just use the native JS/node.js types, which are in the object anyway. More elegant and more efficient.

        if ( typeof message === 'string' ) {
            console.log('Received Message: ' + message);
            connection.sendUTF(message);
        }
        else if ( message instanceof Buffer ) {
            console.log('Received Binary Message of ' + message.length + ' bytes');
            connection.sendBytes(message);
        }
ibc commented 9 years ago

Read RFC 6455 - WebSocket Protocol. WebSocket message type can be UTF-8 or binary, so "any" kind of string is NOT valid.

ghost commented 9 years ago

I think you missed my point. The point is how you represent the data and why not use the built-in types instead of wrapping them in an object which is redundant. Also JavaScript strings are UTF-16, so when you have something like { type: "utf8", utf8Data: "whatever" } this is actually UTF16 internally. I never said anything about "any" kind.

theturtle32 commented 9 years ago

A case can be made for building the API as you describe, but it's not going to make a significant difference in terms of performance. And unfortunately that ship has sailed. The API must be maintained as it is now I order to ensure compatibility with existing code that's using it.

Also, JavaScript is not UTF-16 internally. Last I checked, V8 used UCS-2 as the internal representation, which required the usage of surrogate pairs in order to represent characters outside the BMP.

ibc commented 9 years ago

The API must be maintained as it is now I order to ensure compatibility with existing code that's using it.

VERSION_MAJOR++ :)

theturtle32 commented 9 years ago

Well, yes, but I'm not going to do a major version bump just for that :)