Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dgram: prefer strict equality #8011

Closed
wants to merge 1 commit into from
Closed

dgram: prefer strict equality #8011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/dgram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ function lookup6(address, callback) {


function newHandle(type) {
if (type == 'udp4') {
if (type === 'udp4') {
const handle = new UDP();
handle.lookup = lookup4;
return handle;
}

if (type == 'udp6') {
if (type === 'udp6') {
const handle = new UDP();
handle.lookup = lookup6;
handle.bind = handle.bind6;
Expand Down Expand Up @@ -78,7 +78,7 @@ exports._createSocketHandle = function(address, port, addressType, fd, flags) {
function Socket(type, listener) {
EventEmitter.call(this);

if (typeof type === 'object') {
if (type !== null && typeof type === 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

I think V8 likes it better to have typeof checks come first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax do you have any docs on this? Not saying it's not the case - I don't really know myself, but a quick google search throws nothing, and in the lib codebase both cases seem to be equally prevalent:

node/lib/https.js

Line 53 in be68b68

function createConnection(port, host, options) {

node/lib/dns.js

Line 111 in 013d76c

if (hostname && typeof hostname !== 'string') {

vs

node/lib/querystring.js

Line 172 in 6979632

if (typeof v === 'number' && isFinite(v))

node/lib/events.js

Line 63 in b7a8a69

if (typeof n !== 'number' || n < 0 || isNaN(n))

if we can confirm one case is better, this could mean future improvement PRs and more consistency over the codebase

Copy link
Member

Choose a reason for hiding this comment

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

@claudiorodriguez I’ll try to look it up/benchmark it, all I could remember right now is emberjs/ember.js#14118 … might be a different case with !== null, but even so I think I’d find the typeof-first variants more readable.

Copy link
Member

Choose a reason for hiding this comment

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

@claudiorodriguez This does indeed seem to be different with !== null. You can feel free to disregard my comment :)
Expressions of the form a && typeof a === 'object' definitely seem to be a reason for deopts, though.

var options = type;
type = options.type;
}
Expand Down Expand Up @@ -137,7 +137,7 @@ Socket.prototype.bind = function(port_ /*, address, callback*/) {

self._healthCheck();

if (this._bindState != BIND_STATE_UNBOUND)
if (this._bindState !== BIND_STATE_UNBOUND)
throw new Error('Socket is already bound');

this._bindState = BIND_STATE_BINDING;
Expand Down Expand Up @@ -346,15 +346,15 @@ Socket.prototype.send = function(buffer,

self._healthCheck();

if (self._bindState == BIND_STATE_UNBOUND)
if (self._bindState === BIND_STATE_UNBOUND)
self.bind({port: 0, exclusive: true}, null);

if (list.length === 0)
list.push(Buffer.allocUnsafe(0));

// If the socket hasn't been bound yet, push the outbound packet onto the
// send queue and send after binding is complete.
if (self._bindState != BIND_STATE_BOUND) {
if (self._bindState !== BIND_STATE_BOUND) {
enqueue(self, self.send.bind(self, list, port, address, callback));
return;
}
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-dgram-createSocket-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';
require('../common');
const assert = require('assert');
const dgram = require('dgram');
const invalidTypes = [
'test',
['udp4'],
new String('udp4'),
1,
{},
true,
false,
null,
undefined
];
const validTypes = [
'udp4',
'udp6',
{ type: 'udp4' },
{ type: 'udp6' }
];

// Error must be thrown with invalid types
invalidTypes.forEach((invalidType) => {
assert.throws(() => {
dgram.createSocket(invalidType);
}, /Bad socket type specified/);
});

// Error must not be thrown with valid types
validTypes.forEach((validType) => {
assert.doesNotThrow(() => {
const socket = dgram.createSocket(validType);
socket.close();
});
});