Skip to content

process: Add code to warnings, assign codes to deprecations #10116

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
jasnell wants to merge 3 commits into from
Closed

process: Add code to warnings, assign codes to deprecations #10116

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81db4e8
process: add optional code to warnings + type checking
jasnell Dec 4, 2016
be11fb4
process: add --redirect-warnings command line argument
jasnell Dec 4, 2016
ab50285
lib: add static identifier codes for all deprecations
jasnell Dec 4, 2016
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
Next Next commit
process: add optional code to warnings + type checking 
Add the ability to assign an optional code to process warnings +
add additional type checking to ensure that names and codes can
only be strings.
  • Loading branch information
@jasnell
jasnell committed Jan 30, 2017
commit 81db4e80d2fe0d15b6109ed2ad75ce8508a1df0b
25 changes: 17 additions & 8 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,14 +706,15 @@ console.log(process.env.test);
// => 1
```

## process.emitWarning(warning[, name][, ctor])
## process.emitWarning(warning[, type[, code]][, ctor])

@silverwind silverwind Dec 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think a options object {type, code, ctor} is in order here.

@sam-github sam-github Jan 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is basically just intended for internal use, so doesn't need a nice API, and changing the call signature would cause a fair amount of internal churn.

Honestly, I don't think it should be an API at all, maybe we can deprecate it? Seriously, why do we want to expose this to users of node? We expect them to be using it to emit their own warnings?

@addaleax addaleax Jan 10, 2017

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.

Honestly, I don't think it should be an API at all, maybe we can deprecate it? Seriously, why do we want to expose this to users of node? We expect them to be using it to emit their own warnings?

Fwiw it was deliberately added as an outward-facing API in c6656db (released in v6.0.0) with that intention, yes. I don’t mind it being there.

@jasnell jasnell Jan 10, 2017

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 was exposed this way because we had users asking to be able to use it also. I'm definitely -1 on deprecating it as I have found it to be quite useful

@sam-github sam-github Jan 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, then I agree with @silverwind, its become an ugly API, with three strings in a row, 2 of which are optional, and remembering the arg order isn't reasonably possible. @addaleax the problem with APIs is they rot with time, becoming harder and harder to use and support, like this one is now, which is why I am generally in favour of not adding unnecessary APIs, even if when they are useful. I guess given the alternative of adding a second options-based call pattern, and supporting the old 3 argument call for backwards compatibility, and documenting both of them, or just adding another optional string arg, better to do the latter. If we ever add a 4th string, though, it will no longer be reasonable.

@silverwind silverwind Jan 11, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sam-github My thoughts exactly. Supporting two call signatures is easy and enables easier future additons.

@jasnell jasnell Jan 11, 2017

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.

The API already supports two call signatures. The first argument can be passed in as a pre-constructed Error object with all of the relevant properties set:

const warning = new Error('This is a warning');
warning.name = 'CustomWarning';
warning.code = 'ABC123';
process.emitWarning(warning);

is equivalent to:

process.emitWarning('This is a warning', 'CustomWarning', 'ABC123');

Having an options object when we can already pass in an Error object would be rather pointless, IMO

@sam-github sam-github Jan 11, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That Error behaviour isn't well documented. I read carefully, and I see now that its implied by by the "If warning is passed as an Error object, it will be passed through .... unmodified" sentence.

I think the docs could be clearer, but that's an aside from this PR.

@jasnell jasnell Jan 11, 2017

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's not just implied, it's explicitly stated in the arguments list that warning can be a string or an Error, and an example of passing an Error is shown immediately following the If warning is passed as an Error object... sentence. Immediately following the example is a clear statement that a TypeError will be thrown is warning is anything other than a string or Error object. The docs seem pretty explicit about it.

<!-- YAML
added: v6.0.0
-->

* `warning` {String | Error} The warning to emit.
* `name` {String} When `warning` is a String, `name` is the name to use
for the warning. Default: `Warning`.
* `type` {String} When `warning` is a String, `type` is the name to use
for the *type* of warning being emitted. Default: `Warning`.
* `code` {String} A unique identifier for the warning instance being emitted.

@sam-github sam-github Jan 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think "number" when I think "code", just me? This seems like an "id", its even described as an identifier in the docs.

@jasnell jasnell Jan 10, 2017

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.

Use of code to contain a string is already well established (https://nodejs.org/dist/latest-v7.x/docs/api/errors.html#errors_error_code) and is consistent with existing practice in core.

* `ctor` {Function} When `warning` is a String, `ctor` is an optional
function used to limit the generated stack trace. Default
`process.emitWarning`
Expand All @@ -729,11 +730,16 @@ process.emitWarning('Something happened!');
```

```js
// Emit a warning using a string and a name...
// Emit a warning using a string and a type...
process.emitWarning('Something Happened!', 'CustomWarning');
// Emits: (node:56338) CustomWarning: Something Happened!
```

```js
process.emitWarning('Something happened!', 'CustomWarning', 'WARN001');
// Emits: (node:56338) CustomWarning [WARN001]: Something Happened!
```

In each of the previous examples, an `Error` object is generated internally by
`process.emitWarning()` and passed through to the
[`process.on('warning')`][process_warning] event.
Expand All @@ -742,21 +748,24 @@ In each of the previous examples, an `Error` object is generated internally by
process.on('warning', (warning) => {
console.warn(warning.name);
console.warn(warning.message);
console.warn(warning.code);
console.warn(warning.stack);
});
```

If `warning` is passed as an `Error` object, it will be passed through to the
`process.on('warning')` event handler unmodified (and the optional `name`
and `ctor` arguments will be ignored):
`process.on('warning')` event handler unmodified (and the optional `type`,
`code` and `ctor` arguments will be ignored):

```js
// Emit a warning using an Error object...
const myWarning = new Error('Warning! Something happened!');
// Use the Error name property to specify the type name
myWarning.name = 'CustomWarning';
myWarning.code = 'WARN001';

process.emitWarning(myWarning);
// Emits: (node:56338) CustomWarning: Warning! Something Happened!
// Emits: (node:56338) CustomWarning [WARN001]: Warning! Something Happened!
```

A `TypeError` is thrown if `warning` is anything other than a string or `Error`
Expand All @@ -765,7 +774,7 @@ object.
Note that while process warnings use `Error` objects, the process warning
mechanism is **not** a replacement for normal error handling mechanisms.

The following additional handling is implemented if the warning `name` is
The following additional handling is implemented if the warning `type` is
`DeprecationWarning`:

* If the `--throw-deprecation` command-line flag is used, the deprecation
Expand Down
41 changes: 30 additions & 11 deletions lib/internal/process/warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,45 @@ function setupProcessWarnings() {
const trace = process.traceProcessWarnings ||
(isDeprecation && process.traceDeprecation);
if (trace && warning.stack) {
console.error(`${prefix}${warning.stack}`);
if (warning.code) {
console.error(`${prefix}[${warning.code}] ${warning.stack}`);
} else {
console.error(`${prefix}${warning.stack}`);
}
} else {
var toString = warning.toString;
if (typeof toString !== 'function')
toString = Error.prototype.toString;
console.error(`${prefix}${toString.apply(warning)}`);
const toString =
typeof warning.toString === 'function' ?
warning.toString : Error.prototype.toString;
if (warning.code) {
console.error(
`${prefix}[${warning.code}] ${toString.apply(warning)}`);
} else {
console.error(`${prefix}${toString.apply(warning)}`);
}
}
});
}

// process.emitWarning(error)
// process.emitWarning(str[, name][, ctor])
process.emitWarning = function(warning, name, ctor) {
if (typeof name === 'function') {
ctor = name;
name = 'Warning';
// process.emitWarning(str[, type[, code]][, ctor])
process.emitWarning = function(warning, type, code, ctor) {
if (typeof type === 'function') {
ctor = type;
code = undefined;
type = 'Warning';
}
if (typeof code === 'function') {
ctor = code;
code = undefined;
}
if (code !== undefined && typeof code !== 'string')
throw new TypeError('\'code\' must be a String');
if (type !== undefined && typeof type !== 'string')
throw new TypeError('\'type\' must be a String');
if (warning === undefined || typeof warning === 'string') {
warning = new Error(warning);
warning.name = name || 'Warning';
warning.name = String(type || 'Warning');
if (code !== undefined) warning.code = code;
Error.captureStackTrace(warning, ctor || process.emitWarning);
}
if (!(warning instanceof Error)) {
Expand Down
19 changes: 16 additions & 3 deletions test/parallel/test-process-emitwarning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,21 @@ const util = require('util');
process.on('warning', common.mustCall((warning) => {
assert(warning);
assert(/^(Warning|CustomWarning)/.test(warning.name));
assert(warning.message, 'A Warning');
}, 7));
assert.strictEqual(warning.message, 'A Warning');
if (warning.code) assert.strictEqual(warning.code, 'CODE001');
}, 8));

process.emitWarning('A Warning');
process.emitWarning('A Warning', 'CustomWarning');
process.emitWarning('A Warning', CustomWarning);
process.emitWarning('A Warning', 'CustomWarning', CustomWarning);
process.emitWarning('A Warning', 'CustomWarning', 'CODE001');

function CustomWarning() {
Error.call(this);
this.name = 'CustomWarning';
this.message = 'A Warning';
this.code = 'CODE001';
Error.captureStackTrace(this, CustomWarning);
}
util.inherits(CustomWarning, Error);
Expand All @@ -36,6 +39,16 @@ warningThrowToString.toString = function() {
};
process.emitWarning(warningThrowToString);

// TypeError is thrown on invalid output
// TypeError is thrown on invalid input
assert.throws(() => process.emitWarning(1), TypeError);
assert.throws(() => process.emitWarning({}), TypeError);
assert.throws(() => process.emitWarning(true), TypeError);
assert.throws(() => process.emitWarning([]), TypeError);
assert.throws(() => process.emitWarning('', {}), TypeError);
assert.throws(() => process.emitWarning('', '', {}), TypeError);
assert.throws(() => process.emitWarning('', 1), TypeError);
assert.throws(() => process.emitWarning('', '', 1), TypeError);
assert.throws(() => process.emitWarning('', true), TypeError);
assert.throws(() => process.emitWarning('', '', true), TypeError);
assert.throws(() => process.emitWarning('', []), TypeError);
assert.throws(() => process.emitWarning('', '', []), TypeError);