Skip to content

lib: refactor internal/util #11404

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

Closed
wants to merge 3 commits into from
Closed

lib: refactor internal/util #11404

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf29544
lib: refactor internal/util
jasnell Feb 15, 2017
4b013e8
Address nits
jasnell Apr 20, 2017
3a6954a
Address nits
jasnell Apr 20, 2017
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
114 changes: 74 additions & 40 deletions lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,43 @@ const signals = process.binding('constants').os.signals;

const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol'];
const noCrypto = !process.versions.openssl;

// The `buffer` module uses this. Defining it here instead of in the public
// `util` module makes it accessible without having to `require('util')` there.
exports.customInspectSymbol = Symbol('util.inspect.custom');
/*
* Implementation of ToInteger as per ECMAScript Specification
* Refer: http://www.ecma-international.org/ecma-262/6.0/#sec-tointeger
*/
function toInteger(argument) {
const number = +argument;
return Number.isNaN(number) ? 0 : Math.trunc(number);
}

// Mark that a method should not be used.
// Returns a modified function which warns once by default.
// If --no-deprecation is set, then it is a no-op.
exports.deprecate = function deprecate(fn, msg, code) {
/*
* Implementation of ToLength as per ECMAScript Specification
* Refer: http://www.ecma-international.org/ecma-262/6.0/#sec-tolength
*/
function toLength(argument) {
const len = toInteger(argument);
return len <= 0 ? 0 : Math.min(len, Number.MAX_SAFE_INTEGER);
}

function isError(e) {
return objectToString(e) === '[object Error]' || e instanceof Error;
}

Copy link
Contributor

@sam-github sam-github Apr 20, 2017

Choose a reason for hiding this comment

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

sometimes there is two lines beween definitions, sometimes one, should probably be consisten


function objectToString(o) {
return Object.prototype.toString.call(o);
}

// Mark that a method should not be used.
Copy link
Contributor

@mscdex mscdex Apr 20, 2017

Choose a reason for hiding this comment

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

These comments shouldn't be indented.

Copy link
Member Author

@jasnell jasnell Apr 20, 2017

Choose a reason for hiding this comment

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

whoops! good catch :-)

// Returns a modified function which warns once by default.
// If --no-deprecation is set, then it is a no-op.
function deprecate(fn, msg, code) {
// Allow for deprecating things in the process of starting up.
if (global.process === undefined) {
return function() {
return exports.deprecate(fn, msg, code).apply(this, arguments);
return function(...args) {
return deprecate(fn, msg).apply(this, args);
};
}

Expand All @@ -29,7 +53,7 @@ exports.deprecate = function deprecate(fn, msg, code) {
throw new TypeError('`code` argument must be a string');

var warned = false;
function deprecated() {
function deprecated(...args) {
if (!warned) {
warned = true;
if (code !== undefined) {
Expand All @@ -39,9 +63,9 @@ exports.deprecate = function deprecate(fn, msg, code) {
}
}
if (new.target) {
return Reflect.construct(fn, arguments, new.target);
return Reflect.construct(fn, args, new.target);
}
return fn.apply(this, arguments);
return fn.apply(this, args);
}

// The wrapper will keep the same prototype as fn to maintain prototype chain
Expand All @@ -54,10 +78,10 @@ exports.deprecate = function deprecate(fn, msg, code) {
}

return deprecated;
};
}

exports.decorateErrorStack = function decorateErrorStack(err) {
if (!(exports.isError(err) && err.stack) ||
function decorateErrorStack(err) {
if (!(isError(err) && err.stack) ||
binding.getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true)
return;

Expand All @@ -67,30 +91,19 @@ exports.decorateErrorStack = function decorateErrorStack(err) {
err.stack = arrow + err.stack;
binding.setHiddenValue(err, kDecoratedPrivateSymbolIndex, true);
}
};

exports.isError = function isError(e) {
return exports.objectToString(e) === '[object Error]' || e instanceof Error;
};

exports.objectToString = function objectToString(o) {
return Object.prototype.toString.call(o);
};
}

const noCrypto = !process.versions.openssl;
exports.assertCrypto = function() {
function assertCrypto() {
if (noCrypto)
throw new Error('Node.js is not compiled with openssl crypto support');
};

exports.kIsEncodingSymbol = Symbol('node.isEncoding');
}

// The loop should only run at most twice, retrying with lowercased enc
// if there is no match in the first pass.
// We use a loop instead of branching to retry with a helper
// function in order to avoid the performance hit.
// Return undefined if there is no match.
exports.normalizeEncoding = function normalizeEncoding(enc) {
function normalizeEncoding(enc) {
if (!enc) return 'utf8';
var retried;
while (true) {
Expand All @@ -116,11 +129,9 @@ exports.normalizeEncoding = function normalizeEncoding(enc) {
retried = true;
}
}
};
}

// Filters duplicate strings. Used to support functions in crypto and tls
// modules. Implemented specifically to maintain existing behaviors in each.
exports.filterDuplicateStrings = function filterDuplicateStrings(items, low) {
function filterDuplicateStrings(items, low) {
const map = new Map();
for (var i = 0; i < items.length; i++) {
const item = items[i];
Expand All @@ -132,24 +143,24 @@ exports.filterDuplicateStrings = function filterDuplicateStrings(items, low) {
}
}
return Array.from(map.values()).sort();
};
}

exports.cachedResult = function cachedResult(fn) {
function cachedResult(fn) {
var result;
return () => {
if (result === undefined)
result = fn();
return result.slice();
};
};
}

// Useful for Wrapping an ES6 Class with a constructor Function that
// does not require the new keyword. For instance:
// class A { constructor(x) {this.x = x;}}
// const B = createClassWrapper(A);
// B() instanceof A // true
// B() instanceof B // true
exports.createClassWrapper = function createClassWrapper(type) {
function createClassWrapper(type) {
const fn = function(...args) {
return Reflect.construct(type, args, new.target || type);
};
Expand All @@ -161,7 +172,7 @@ exports.createClassWrapper = function createClassWrapper(type) {
Object.setPrototypeOf(fn, type);
fn.prototype = type.prototype;
return fn;
};
}

let signalsToNamesMapping;
function getSignalsToNamesMapping() {
Expand All @@ -176,7 +187,7 @@ function getSignalsToNamesMapping() {
return signalsToNamesMapping;
}

exports.convertToValidSignal = function convertToValidSignal(signal) {
function convertToValidSignal(signal) {
if (typeof signal === 'number' && getSignalsToNamesMapping()[signal])
return signal;

Expand All @@ -186,4 +197,27 @@ exports.convertToValidSignal = function convertToValidSignal(signal) {
}

throw new Error('Unknown signal: ' + signal);
}

module.exports = {
Copy link
Contributor

@sam-github sam-github Apr 20, 2017

Choose a reason for hiding this comment

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

assigning to exports = as well is more future proof, it allows util functions to refer to each other more easily.

Of the 3 common export styles, at-top, at-definition (what util used to do), and at-bottom,at-bottom remains my least favorite. It lacks the visibility and readability of export at top, and lacks the cohesion of export at time of definition. But if at-bottom is how node.js goes, I'll follow along.

Copy link
Member Author

@jasnell jasnell Apr 20, 2017

Choose a reason for hiding this comment

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

Avoiding the need to refer to exports.{whatever} is important and is why the various exported functions are all named top level functions (they can refer to each other without the exports. bit). That said, it's harmless to add.

Copy link
Contributor

@mscdex mscdex Apr 20, 2017

Choose a reason for hiding this comment

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

One problem with having it at the top is that you can run into variable definition issues, where you're exporting something that gets defined/set later on in the file. Sometimes you might be able to just pull the definition to the top, but other times the assignment may not be a simple one-liner. So putting it at the bottom means you never have to worry about such issues and doing it the same everywhere is good for consistency.

assertCrypto,
cachedResult,
convertToValidSignal,
createClassWrapper,
decorateErrorStack,
deprecate,
filterDuplicateStrings,
isError,
normalizeEncoding,
objectToString,
toInteger,
toLength,

// Symbol used to provide a custom inspect function for an object as an
// alternative to using 'inspect'
customInspectSymbol: Symbol('util.inspect.custom'),

// Used by the buffer module to capture an internal reference to the
// default isEncoding implementation, just in case userland overrides it.
kIsEncodingSymbol: Symbol('node.isEncoding')
};