Refactor `WebSocket` to use class syntax #930

lpinca · 2016-12-09T10:16:28Z

Refactor WebSocket to use the ES6 class syntax.
Move cleanupWebsocketResources and establishConnection to WebSocket.prototype.
Rename establishConnection to setSocket and cleanupWebsocketResources to finalize.
Keep initAsServerClient and initAsClient as completely private API.

lpinca · 2016-12-09T13:50:53Z

I think the new methods names (setSocket and finalize) are better than the original ones but I'm not very happy with them. If you have better ideas please share!

ericmdantas · 2016-12-09T14:04:01Z

lib/WebSocket.js

+   * @type {Number}
+   */
+  get bufferedAmount () {
+    var amount = 0;


Not yet as it causes deoptimizations.

I thought only compound let statements caused deoptimization?
source: http://stackoverflow.com/questions/34595356/what-does-compound-let-const-assignment-mean

I think it's still better to keep var as it is consistent with the rest of the code base and we are 100% safe.
Switching to let should be very easy with an eslint rule when we are sure it doesn't cause any harm.

Makes sense. I wish there was documentation somewhere for v8's optimizations. It would make things like this so much easier.

FWIW nodejs/node#9729 (comment)

ericmdantas · 2016-12-09T14:07:28Z

lib/WebSocket.js

-    cleanupWebsocketResources.call(this, true);
-  }
-};
+    if (!data) data = '';


Use default param value instead?

Does not work on Node 4.

@lpinca Now that node 6 is the new LTS, maybe we should use that our minimum which also allows us to use default params.

@3rd-Eden I think that dropping Node 4 support is a bit too much. For example AWS Lambda does not support Node 6 yet.

Now using ws with Lambda does not make much sense 😄 but I think you get my point.

@lpinca ok, we'll wait another week. Or until node 4 LTS runs out completely.

Or until node 4 LTS runs out completely.

Yeah that's my idea but that will not happen before April 2017 and after that there is 1 more year of maintenance.

Maybe use === undefined to mirror the behavior of default parameters, so that making the eventual switch doesn't cause any unintended side effects?

@JoshuaWise no strong opinion on this but for example

ws/lib/Sender.js

Lines 154 to 163 in 5a8ead5

if (data && !Buffer.isBuffer(data)) {

if (data instanceof ArrayBuffer) {

data = Buffer.from(data);

} else if (ArrayBuffer.isView(data)) {

data = viewToBuffer(data);

} else {

data = Buffer.from(typeof data === 'number' ? data.toString() : data);

readOnly = false;

}

}

just check that data is truthy.

We should probably also allow 0 which doesn't work atm, so this makes sense but maybe it's better to fix the issue in another PR/commit as we should fix the edge case also in Sender.

ericmdantas · 2016-12-09T14:07:51Z

lib/WebSocket.js

+      throw new Error('not opened');
+    }
+
+    options = options || {};


Use default param value instead?

lpinca · 2016-12-09T14:13:12Z

@ericmdantas thank you for reviewing, this is greatly appreciated! Let me know if you have more suggestions.

ericmdantas · 2016-12-09T14:48:16Z

@lpinca, no problem. I'm a bit busy at the moment, but I'll take a closer look when possible. But, from what I've seen, it's good to go.

lpinca · 2016-12-14T07:29:04Z

I'll merge this later today if there are no objections.

lpinca force-pushed the use/class-syntax branch from ce4c369 to 16e4656 Compare December 9, 2016 13:40

ericmdantas reviewed Dec 9, 2016

View reviewed changes

lib/WebSocket.js

throw new Error('not opened');

}

options = options || {};

Copy link

ericmdantas Dec 9, 2016

Choose a reason for hiding this comment

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

Use default param value instead?

[major] Refactor WebSocket to use class syntax

6e4f364

lpinca force-pushed the use/class-syntax branch from 16e4656 to 6e4f364 Compare December 10, 2016 19:32

lpinca merged commit 099c636 into master Dec 14, 2016

lpinca deleted the use/class-syntax branch December 14, 2016 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `WebSocket` to use class syntax #930

Refactor `WebSocket` to use class syntax #930

lpinca commented Dec 9, 2016 •

edited

Loading

lpinca commented Dec 9, 2016

ericmdantas Dec 9, 2016

lpinca Dec 9, 2016

JoshuaWise Dec 14, 2016

lpinca Dec 14, 2016

JoshuaWise Dec 14, 2016

lpinca Dec 14, 2016

ericmdantas Dec 9, 2016

lpinca Dec 9, 2016

3rd-Eden Dec 9, 2016

lpinca Dec 9, 2016

3rd-Eden Dec 9, 2016

lpinca Dec 9, 2016

JoshuaWise Dec 14, 2016

lpinca Dec 14, 2016

lpinca Dec 14, 2016

ericmdantas Dec 9, 2016

lpinca commented Dec 9, 2016

ericmdantas commented Dec 9, 2016

lpinca commented Dec 14, 2016

	if (data && !Buffer.isBuffer(data)) {
	if (data instanceof ArrayBuffer) {
	data = Buffer.from(data);
	} else if (ArrayBuffer.isView(data)) {
	data = viewToBuffer(data);
	} else {
	data = Buffer.from(typeof data === 'number' ? data.toString() : data);
	readOnly = false;
	}
	}

Refactor WebSocket to use class syntax #930

Refactor WebSocket to use class syntax #930

Conversation

lpinca commented Dec 9, 2016 • edited Loading

lpinca commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Dec 9, 2016

ericmdantas commented Dec 9, 2016

lpinca commented Dec 14, 2016

Refactor `WebSocket` to use class syntax #930

Refactor `WebSocket` to use class syntax #930

lpinca commented Dec 9, 2016 •

edited

Loading