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.

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

lib: refactor some internal/* code #12644

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const binding = process.binding('http_parser');
const methods = binding.methods;
const HTTPParser = binding.HTTPParser;

const FreeList = require('internal/freelist').FreeList;
const FreeList = require('internal/freelist');
const ondrain = require('internal/http').ondrain;
const incoming = require('_http_incoming');
const IncomingMessage = incoming.IncomingMessage;
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/freelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ class FreeList {
}
}

module.exports = {FreeList};
module.exports = FreeList;
21 changes: 11 additions & 10 deletions lib/internal/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@ const Buffer = require('buffer').Buffer;
const Writable = require('stream').Writable;
const fs = require('fs');
const util = require('util');
const constants = process.binding('constants').fs;

const O_APPEND = constants.O_APPEND | 0;
Copy link
Contributor

@mscdex mscdex Apr 25, 2017

Choose a reason for hiding this comment

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

Might want to check if the implicit coercion to a 32-bit value was intentional (e.g. performance related or the values were not originally numbers for some reason)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I explored that a bit a couldn't find anything specific. I could be wrong, but I think the switch was more defensive just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also converts to an integer if it was a float, or some other type of number-ish.

const O_CREAT = constants.O_CREAT | 0;
const O_EXCL = constants.O_EXCL | 0;
const O_RDONLY = constants.O_RDONLY | 0;
const O_RDWR = constants.O_RDWR | 0;
const O_SYNC = constants.O_SYNC | 0;
const O_TRUNC = constants.O_TRUNC | 0;
const O_WRONLY = constants.O_WRONLY | 0;

const {
O_APPEND,
O_CREAT,
O_EXCL,
O_RDONLY,
O_RDWR,
O_SYNC,
O_TRUNC,
O_WRONLY
} = process.binding('constants').fs;

function assertEncoding(encoding) {
if (encoding && !Buffer.isEncoding(encoding)) {
Expand Down
22 changes: 11 additions & 11 deletions lib/internal/module.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
'use strict';

exports = module.exports = {
makeRequireFunction,
stripBOM,
stripShebang,
addBuiltinLibsToObject
};

exports.requireDepth = 0;

// Invoke with makeRequireFunction(module) where |module| is the Module object
// to use as the context for the require() function.
function makeRequireFunction(mod) {
Expand Down Expand Up @@ -85,7 +76,7 @@ function stripShebang(content) {
return content;
}

exports.builtinLibs = [
const builtinLibs = [
'assert', 'buffer', 'child_process', 'cluster', 'crypto', 'dgram', 'dns',
'domain', 'events', 'fs', 'http', 'https', 'net', 'os', 'path', 'punycode',
'querystring', 'readline', 'repl', 'stream', 'string_decoder', 'tls', 'tty',
Expand All @@ -94,7 +85,7 @@ exports.builtinLibs = [

function addBuiltinLibsToObject(object) {
// Make built-in modules available directly (loaded lazily).
exports.builtinLibs.forEach((name) => {
builtinLibs.forEach((name) => {
// Goals of this mechanism are:
// - Lazy loading of built-in modules
// - Having all built-in modules available as non-enumerable properties
Expand Down Expand Up @@ -130,3 +121,12 @@ function addBuiltinLibsToObject(object) {
});
});
}

module.exports = exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove exports?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's my preference but in a separate issue @sam-github expressed a preference for these to retain the = exports

Copy link
Contributor

@Fishrock123 Fishrock123 Apr 27, 2017

Choose a reason for hiding this comment

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

Why? There is no point. Since you are already exposing the variable name to grab it this way exports is defunct. (Edit: because You can just access the variable anyways)

If necessary, we can remove this in another PR entirely but idk why we'd add more here...

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree that re-assigning exports is unnecessary. *shrug*

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm perfectly happy with removing it.

addBuiltinLibsToObject,
builtinLibs,
makeRequireFunction,
requireDepth: 0,
stripBOM,
stripShebang
};
21 changes: 11 additions & 10 deletions lib/internal/process.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,6 @@ function lazyConstants() {
return _lazyConstants;
}

exports.setup_cpuUsage = setup_cpuUsage;
exports.setup_hrtime = setup_hrtime;
exports.setupMemoryUsage = setupMemoryUsage;
exports.setupConfig = setupConfig;
exports.setupKillAndExit = setupKillAndExit;
exports.setupSignalHandlers = setupSignalHandlers;
exports.setupChannel = setupChannel;
exports.setupRawDebug = setupRawDebug;


const assert = process.assert = function(x, msg) {
if (!x) throw new Error(msg || 'assertion error');
};
Expand Down Expand Up @@ -267,3 +257,14 @@ function setupRawDebug() {
rawDebug(format.apply(null, arguments));
};
}

module.exports = {
setup_cpuUsage,
setup_hrtime,
setupMemoryUsage,
setupConfig,
setupKillAndExit,
setupSignalHandlers,
setupChannel,
setupRawDebug
};
116 changes: 58 additions & 58 deletions lib/internal/streams/BufferList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,71 +2,71 @@

const Buffer = require('buffer').Buffer;

module.exports = BufferList;
module.exports = class BufferList {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you benchmark this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, negligible difference in master with 5.7. Notable improvement under TF-I ... tho I'd have to pull the numbers back up. It was a week or so ago when I ran it.

constructor() {
this.head = null;
this.tail = null;
this.length = 0;
}

function BufferList() {
this.head = null;
this.tail = null;
this.length = 0;
}
push(v) {
const entry = { data: v, next: null };
if (this.length > 0)
this.tail.next = entry;
else
this.head = entry;
this.tail = entry;
++this.length;
}

BufferList.prototype.push = function(v) {
const entry = { data: v, next: null };
if (this.length > 0)
this.tail.next = entry;
else
unshift(v) {
const entry = { data: v, next: this.head };
if (this.length === 0)
this.tail = entry;
this.head = entry;
this.tail = entry;
++this.length;
};
++this.length;
}

BufferList.prototype.unshift = function(v) {
const entry = { data: v, next: this.head };
if (this.length === 0)
this.tail = entry;
this.head = entry;
++this.length;
};
shift() {
if (this.length === 0)
return;
const ret = this.head.data;
if (this.length === 1)
this.head = this.tail = null;
else
this.head = this.head.next;
--this.length;
return ret;
}

BufferList.prototype.shift = function() {
if (this.length === 0)
return;
const ret = this.head.data;
if (this.length === 1)
clear() {
this.head = this.tail = null;
else
this.head = this.head.next;
--this.length;
return ret;
};

BufferList.prototype.clear = function() {
this.head = this.tail = null;
this.length = 0;
};
this.length = 0;
}

BufferList.prototype.join = function(s) {
if (this.length === 0)
return '';
var p = this.head;
var ret = '' + p.data;
while (p = p.next)
ret += s + p.data;
return ret;
};
join(s) {
if (this.length === 0)
return '';
var p = this.head;
var ret = '' + p.data;
while (p = p.next)
ret += s + p.data;
return ret;
}

BufferList.prototype.concat = function(n) {
if (this.length === 0)
return Buffer.alloc(0);
if (this.length === 1)
return this.head.data;
const ret = Buffer.allocUnsafe(n >>> 0);
var p = this.head;
var i = 0;
while (p) {
p.data.copy(ret, i);
i += p.data.length;
p = p.next;
concat(n) {
if (this.length === 0)
return Buffer.alloc(0);
if (this.length === 1)
return this.head.data;
const ret = Buffer.allocUnsafe(n >>> 0);
var p = this.head;
var i = 0;
while (p) {
p.data.copy(ret, i);
i += p.data.length;
p = p.next;
}
return ret;
}
return ret;
};
7 changes: 3 additions & 4 deletions test/parallel/test-freelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

require('../common');
const assert = require('assert');
const freelist = require('internal/freelist');
const FreeList = require('internal/freelist');

assert.strictEqual(typeof freelist, 'object');
assert.strictEqual(typeof freelist.FreeList, 'function');
assert.strictEqual(typeof FreeList, 'function');

const flist1 = new freelist.FreeList('flist1', 3, String);
const flist1 = new FreeList('flist1', 3, String);

// Allocating when empty, should not change the list size
const result = flist1.alloc('test');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-internal-modules-expose.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ const config = process.binding('config');

console.log(config, process.argv);

assert.strictEqual(typeof require('internal/freelist').FreeList, 'function');
assert.strictEqual(typeof require('internal/freelist'), 'function');
assert.strictEqual(config.exposeInternals, true);