Skip to content

lib: hide lots of internal properties #23126

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
BridgeAR wants to merge 1 commit into from
Closed

lib: hide lots of internal properties #23126

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
16 changes: 14 additions & 2 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,20 @@ function Server(options, requestListener) {
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
}

this[kIncomingMessage] = options.IncomingMessage || IncomingMessage;
this[kServerResponse] = options.ServerResponse || ServerResponse;
Object.defineProperties(this, {

@joyeecheung joyeecheung Dec 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it would be worthwhile to put these into a helper and then document about util.inspect.defaultOptions in the definition of that helper because to understand what those properties are one has to look into the source code anyway

@BridgeAR BridgeAR Dec 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Shall I do this in this specific PR or is that fine for a separate one?

@mcollina mcollina Dec 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it should be in this PR.

[kIncomingMessage]: {
value: options.IncomingMessage || IncomingMessage,
configurable: true,
writable: true,
enumerable: false
},
[kServerResponse]: {
value: options.ServerResponse || ServerResponse,
configurable: true,
writable: true,
enumerable: false
},
});

net.Server.call(this, { allowHalfOpen: true });

Expand Down
15 changes: 11 additions & 4 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,17 @@ class AsyncResource {
throw new ERR_INVALID_ASYNC_ID('triggerAsyncId', triggerAsyncId);
}

this[async_id_symbol] = newAsyncId();
this[trigger_async_id_symbol] = triggerAsyncId;
// this prop name (destroyed) has to be synchronized with C++
this[destroyedSymbol] = { destroyed: false };
const attributes = {
configurable: true,
writable: true,
enumerable: false
};
Object.defineProperties(this, {
[async_id_symbol]: { ...attributes, value: newAsyncId() },
[trigger_async_id_symbol]: { ...attributes, value: triggerAsyncId },
// This prop name (destroyed) has to be synchronized with C++
[destroyedSymbol]: { ...attributes, value: { destroyed: false } }
});

emitInit(
this[async_id_symbol], type, this[trigger_async_id_symbol], this
Expand Down
14 changes: 11 additions & 3 deletions lib/inspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,17 @@ const onMessageSymbol = Symbol('onMessage');
class Session extends EventEmitter {
constructor() {
super();
this[connectionSymbol] = null;
this[nextIdSymbol] = 1;
this[messageCallbacksSymbol] = new Map();

const attributes = {
configurable: true,
writable: true,
enumerable: false
};
Object.defineProperties(this, {
[connectionSymbol]: { ...attributes, value: null },
[nextIdSymbol]: { ...attributes, value: 1 },
[messageCallbacksSymbol]: { ...attributes, value: new Map() }
});
}

connect() {
Expand Down
67 changes: 30 additions & 37 deletions lib/internal/console/constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,8 @@ let cliTable;

// Track amount of indentation required via `console.group()`.
const kGroupIndent = Symbol('kGroupIndent');
const kFormatForStderr = Symbol('kFormatForStderr');
const kFormatForStdout = Symbol('kFormatForStdout');
const kGetInspectOptions = Symbol('kGetInspectOptions');
const kColorMode = Symbol('kColorMode');
const kIsConsole = Symbol('kIsConsole');
const kWriteToConsole = Symbol('kWriteToConsole');
const kBindProperties = Symbol('kBindProperties');
const kBindStreamsEager = Symbol('kBindStreamsEager');
const kBindStreamsLazy = Symbol('kBindStreamsLazy');
Expand Down Expand Up @@ -163,15 +159,14 @@ Console.prototype[kBindProperties] = function(ignoreErrors, colorMode) {
...consolePropAttributes,
value: Boolean(ignoreErrors)
},
'_times': { ...consolePropAttributes, value: new Map() }
'_times': { ...consolePropAttributes, value: new Map() },
// TODO(joyeecheung): use consolePropAttributes for these
// Corresponds to https://console.spec.whatwg.org/#count-map
[kCounts]: { ...consolePropAttributes, value: new Map() },
[kColorMode]: { ...consolePropAttributes, value: colorMode },
[kIsConsole]: { ...consolePropAttributes, value: true },
[kGroupIndent]: { ...consolePropAttributes, value: '' },
});

// TODO(joyeecheung): use consolePropAttributes for these
// Corresponds to https://console.spec.whatwg.org/#count-map
this[kCounts] = new Map();
this[kColorMode] = colorMode;
this[kIsConsole] = true;
this[kGroupIndent] = '';
};

// Make a function that can serve as the callback passed to `stream.write()`.
Expand All @@ -195,14 +190,14 @@ function createWriteErrorHandler(instance, streamSymbol) {
};
}

Console.prototype[kWriteToConsole] = function(streamSymbol, string) {
const ignoreErrors = this._ignoreErrors;
const groupIndent = this[kGroupIndent];
function writeToConsole(self, streamSymbol, string) {
const ignoreErrors = self._ignoreErrors;
const groupIndent = self[kGroupIndent];

const useStdout = streamSymbol === kUseStdout;
const stream = useStdout ? this._stdout : this._stderr;
const stream = useStdout ? self._stdout : self._stderr;
const errorHandler = useStdout ?
this._stdoutErrorHandler : this._stderrErrorHandler;
self._stdoutErrorHandler : self._stderrErrorHandler;

if (groupIndent.length !== 0) {
if (string.indexOf('\n') !== -1) {
Expand Down Expand Up @@ -231,50 +226,48 @@ Console.prototype[kWriteToConsole] = function(streamSymbol, string) {
} finally {
stream.removeListener('error', noop);
}
};
}

const kColorInspectOptions = { colors: true };
const kNoColorInspectOptions = {};
Console.prototype[kGetInspectOptions] = function(stream) {
let color = this[kColorMode];

function getInspectOptions(self, stream) {
let color = self[kColorMode];
if (color === 'auto') {
color = stream.isTTY && (
typeof stream.getColorDepth === 'function' ?
stream.getColorDepth() > 2 : true);
}

return color ? kColorInspectOptions : kNoColorInspectOptions;
};

Console.prototype[kFormatForStdout] = function(args) {
const opts = this[kGetInspectOptions](this._stdout);
return util.formatWithOptions(opts, ...args);
};
}

Console.prototype[kFormatForStderr] = function(args) {
const opts = this[kGetInspectOptions](this._stderr);
function format(self, prop, args) {
const opts = getInspectOptions(self, self[prop]);
return util.formatWithOptions(opts, ...args);
};
}

Console.prototype.log = function log(...args) {
this[kWriteToConsole](kUseStdout, this[kFormatForStdout](args));
writeToConsole(this, kUseStdout, format(this, '_stdout', args));
};

Console.prototype.debug = Console.prototype.log;
Console.prototype.info = Console.prototype.log;
Console.prototype.dirxml = Console.prototype.log;

Console.prototype.warn = function warn(...args) {
this[kWriteToConsole](kUseStderr, this[kFormatForStderr](args));
writeToConsole(this, kUseStderr, format(this, '_stderr', args));
};

Console.prototype.error = Console.prototype.warn;

Console.prototype.dir = function dir(object, options) {
options = Object.assign({
customInspect: false
}, this[kGetInspectOptions](this._stdout), options);
this[kWriteToConsole](kUseStdout, util.inspect(object, options));
options = {
customInspect: false,
...getInspectOptions(this, this._stdout),
...options
};
writeToConsole(this, kUseStdout, util.inspect(object, options));
};

Console.prototype.time = function time(label = 'default') {
Expand Down Expand Up @@ -325,7 +318,7 @@ function timeLogImpl(self, name, label, data) {
Console.prototype.trace = function trace(...args) {
const err = {
name: 'Trace',
message: this[kFormatForStderr](args)
message: format(this, '_stderr', args)
};
Error.captureStackTrace(err, trace);
this.error(err.stack);
Expand Down Expand Up @@ -414,7 +407,7 @@ Console.prototype.table = function(tabularData, properties) {
if (v !== null && typeof v === 'object' &&
!isArray(v) && ObjectKeys(v).length > 2)
opt.depth = -1;
Object.assign(opt, this[kGetInspectOptions](this._stdout));
Object.assign(opt, getInspectOptions(this, this._stdout));
return util.inspect(v, opt);
};
const getIndexArray = (length) => ArrayFrom({ length }, (_, i) => inspect(i));
Expand Down
33 changes: 21 additions & 12 deletions lib/internal/crypto/hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,20 @@ const { normalizeEncoding } = require('internal/util');
const { isArrayBufferView } = require('internal/util/types');
const LazyTransform = require('internal/streams/lazy_transform');
const kState = Symbol('kState');
const kFinalized = Symbol('kFinalized');

function Hash(algorithm, options) {
if (!(this instanceof Hash))
return new Hash(algorithm, options);
validateString(algorithm, 'algorithm');
this[kHandle] = new _Hash(algorithm);
this[kState] = {
[kFinalized]: false
};
Object.defineProperty(this, kState, {
value: {
finalized: false
},
configurable: true,
writable: true,
enumerable: false
});

@addaleax addaleax Sep 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a pretty cheap entry and useful as debugging information, I’d keep it.

@BridgeAR BridgeAR Sep 28, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is always possible to view the entry when using console.log(util.inspect(obj, { showHidden: true })). So I would rather keep it hidden from regular users and Node core developers should likely use that command to inspect things / define util.inspect.defaultOptions.showHidden = true in the REPL.

But I don't have a strong opinion about it and if you think it's best to be visible for everyone, I'll change it back.

@addaleax addaleax Oct 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I would rather keep it hidden from regular users and Node core developers should likely use that command to inspect things / define util.inspect.defaultOptions.showHidden = true in the REPL.

What I meant, was: A lot of the information being hidden here is useful not only to Node.js core developers, even if we don’t encourage people to access it. That’s the point. :)

@BridgeAR BridgeAR Dec 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

May I ask in what way this information is valuable if it should not be accessed?

@addaleax addaleax Dec 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May I ask in what way this information is valuable if it should not be accessed?

Being able to inspect things for debugging, and to access them programmatically are pretty different things and I wouldn’t compare them?

LazyTransform.call(this, options);
}

Expand All @@ -53,7 +57,7 @@ Hash.prototype._flush = function _flush(callback) {

Hash.prototype.update = function update(data, encoding) {
const state = this[kState];
if (state[kFinalized])
if (state.finalized)
throw new ERR_CRYPTO_HASH_FINALIZED();

if (typeof data !== 'string' && !isArrayBufferView(data)) {
Expand All @@ -69,15 +73,15 @@ Hash.prototype.update = function update(data, encoding) {

Hash.prototype.digest = function digest(outputEncoding) {
const state = this[kState];
if (state[kFinalized])
if (state.finalized)
throw new ERR_CRYPTO_HASH_FINALIZED();
outputEncoding = outputEncoding || getDefaultEncoding();
if (normalizeEncoding(outputEncoding) === 'utf16le')
throw new ERR_CRYPTO_HASH_DIGEST_NO_UTF16();

// Explicit conversion for backward compatibility.
const ret = this[kHandle].digest(`${outputEncoding}`);
state[kFinalized] = true;
state.finalized = true;
return ret;
};

Expand All @@ -94,9 +98,14 @@ function Hmac(hmac, key, options) {
}
this[kHandle] = new _Hmac();
this[kHandle].init(hmac, toBuf(key));
this[kState] = {
[kFinalized]: false
};
Object.defineProperty(this, kState, {
value: {
finalized: false
},
configurable: true,
writable: true,
enumerable: false
});
LazyTransform.call(this, options);
}

Expand All @@ -110,14 +119,14 @@ Hmac.prototype.digest = function digest(outputEncoding) {
if (normalizeEncoding(outputEncoding) === 'utf16le')
throw new ERR_CRYPTO_HASH_DIGEST_NO_UTF16();

if (state[kFinalized]) {
if (state.finalized) {
const buf = Buffer.from('');
return outputEncoding === 'buffer' ? buf : buf.toString(outputEncoding);
}

// Explicit conversion for backward compatibility.
const ret = this[kHandle].digest(`${outputEncoding}`);
state[kFinalized] = true;
state.finalized = true;
return ret;
};

Expand Down
26 changes: 19 additions & 7 deletions lib/internal/encoding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,12 @@ function getEncodingFromLabel(label) {

class TextEncoder {
constructor() {
this[kEncoder] = true;
Object.defineProperty(this, kEncoder, {
value: true,
configurable: true,
writable: true,
enumerable: false
});
}

get encoding() {
Expand Down Expand Up @@ -437,12 +442,19 @@ function makeTextDecoderJS() {
flags |= options.ignoreBOM ? CONVERTER_FLAGS_IGNORE_BOM : 0;
}

this[kDecoder] = true;
// StringDecoder will normalize WHATWG encoding to Node.js encoding.
this[kHandle] = new (lazyStringDecoder())(enc);
this[kFlags] = flags;
this[kEncoding] = enc;
this[kBOMSeen] = false;
const attributes = {
configurable: true,
writable: true,
enumerable: false
};
Object.defineProperties(this, {
[kDecoder]: { ...attributes, value: true },
// StringDecoder will normalize WHATWG encoding to Node.js encoding.
[kHandle]: { ...attributes, value: new (lazyStringDecoder())(enc) },
[kFlags]: { ...attributes, value: flags },
[kEncoding]: { ...attributes, value: enc },
[kBOMSeen]: { ...attributes, value: false }
});
}

decode(input = empty, options = {}) {
Expand Down
7 changes: 6 additions & 1 deletion lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ const getDirectoryEntriesPromise = promisify(getDirents);

class FileHandle {
constructor(filehandle) {
this[kHandle] = filehandle;
Object.defineProperty(this, kHandle, {
value: filehandle,
configurable: true,
writable: true,
enumerable: false
});
}

getAsyncId() {
Expand Down
14 changes: 12 additions & 2 deletions lib/internal/fs/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ function assertEncoding(encoding) {
class Dirent {
constructor(name, type) {
this.name = name;
this[kType] = type;
Object.defineProperty(this, kType, {
value: type,
configurable: true,
writable: true,
enumerable: false
});
}

isDirectory() {
Expand Down Expand Up @@ -99,7 +104,12 @@ class Dirent {
class DirentFromStats extends Dirent {
constructor(name, stats) {
super(name, null);
this[kStats] = stats;
Object.defineProperty(this, kStats, {
value: stats,
configurable: true,
writable: true,
enumerable: false
});
}
}

Expand Down
16 changes: 14 additions & 2 deletions lib/internal/fs/watchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,20 @@ function StatWatcher(bigint) {
EventEmitter.call(this);

this._handle = null;
this[kOldStatus] = -1;
this[kUseBigint] = bigint;
Object.defineProperties(this, {
[kOldStatus]: {
value: -1,
configurable: true,
writable: true,
enumerable: false
},
[kUseBigint]: {
value: bigint,
configurable: true,
writable: true,
enumerable: false
}

@addaleax addaleax Oct 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess my main issue with these kinds of simple properties that are now hidden is that there’s no way to tell their state from the default util.inspect() output…

@BridgeAR BridgeAR Dec 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well... I am not really sure what to say here. The whole reason for this PR to do exactly that: hide these properties from regular inspection while still keeping the possible to inspect them if done explicitly by using showHidden: true.

});
}
util.inherits(StatWatcher, EventEmitter);

Expand Down
Loading