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 internal/util #11404

Closed
wants to merge 3 commits into from
Closed
Changes from 2 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
93 changes: 53 additions & 40 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,24 @@ 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');
function isError(e) {
return objectToString(e) === '[object Error]' || e instanceof Error;
}

Copy link
Contributor

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

// 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) {
function objectToString(o) {
return Object.prototype.toString.call(o);
}

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

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

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 +34,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 +44,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 +59,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 +72,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 +110,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 +124,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 +153,7 @@ exports.createClassWrapper = function createClassWrapper(type) {
Object.setPrototypeOf(fn, type);
fn.prototype = type.prototype;
return fn;
};
}

let signalsToNamesMapping;
function getSignalsToNamesMapping() {
Expand All @@ -176,7 +168,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 +178,25 @@ exports.convertToValidSignal = function convertToValidSignal(signal) {
}

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

module.exports = {
Copy link
Contributor

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

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

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,

// 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')
};