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

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

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 4, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

process, lib

Description of change

As discussed at the Node.js Interactive Collaboration Summit in Austin last week, this PR adds a few new capabilities that should make it easier to make dealing with runtime deprecation warnings in Node.js easier.

  1. A static identifier code can be assigned to all process warnings. These are specified as either a new optional argument to process.emitWarning() or as a code property on an Error object passed into process.emitWarning(). Within the process.on('warning') event handler, these can be accessed via the code property on the warning object.

  2. A new --redirect-warnings=file command line argument and matching NODE_REDIRECT_WARNINGS=file environment variable is added to allow process warning output to be printed to the given file as opposed to dumping to stderr. This will allow, for instance, npm and other applications spawning node process instances to capture warning output with less noise to the console.

  3. Every runtime and documentation-only deprecation in Node.js is assigned a static identifier code.

  4. A new deprecations.md file is added to the API documentation that lists each of the runtime and documentation-only deprecation codes assigned. For future work (perhaps a good first contribution for new Node.js contributors), it would be helpful to expand the details in deprecations.md to include links to the relevant sections in other parts of the API documentation as well as expanded descriptions of why the APIs are deprecated and what the alternatives are. /cc @nodejs/documentation

Note: This PR is currently semver-major because it adds improved type checking to the input on process.emitWarning() to ensure that both the warning name and the identifier code are strings.
If this PR is backported to older Node.js release streams, the backport can be landed as a semver-minor if and only if the additional type checking on the name argument is removed.

/cc @ashleygwilliams

@jasnell jasnell added lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 4, 2016
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 4, 2016
@jasnell
Copy link
Member Author

jasnell commented Dec 4, 2016

-->

Write process warnings to the given file instead of printing to stderr but
only if the file does not already exist.
Copy link
Member

Choose a reason for hiding this comment

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

Why “only if the file does not already exist”? I’m not sure I’d expect that, and it sounds a bit like that might interfere with programmatic use of the API.

Is this about multiple node processes sharing the same env var?

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 not entirely sure about the only if the file does not already exist part either. For now I have it this way just to ensure that something important isn't accidentally overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just append?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just appending works also. Doing so is pretty safe when dealing with multiple node processes since the warning output includes the pid

`OutgoingMessage.prototype.flushHeaders()` instead.

<a id="DEP00002"></a>
### DEP00002: require('_linklist')
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be \_linklist (including the \)

<!-- 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`.
* `code` {String} A unique identifier for the warning.
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure the semantic difference between name and code is obvious here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Name may be better as "type"?

Copy link
Member Author

Choose a reason for hiding this comment

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

type is a good option

Copy link
Member

Choose a reason for hiding this comment

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

Name may be better as "type"?

Remember that .name is inherited from the Error classes – I don’t think we can change that.

I’m also not worried that much about the naming, a bit of clarification might be all that’s needed

Copy link
Member Author

Choose a reason for hiding this comment

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

The argument can be called type with name still used on the error object. I'm not overly concerned about the difference there.

* The Buffer() construtor is deprecated in documentation and should not be
* used moving forward. Rather, developers should use one of the three new
* factory APIs: Buffer.from(), Buffer.allocUnsafe() or Buffer.alloc() based on
* their specific needs. There is no hard deprecation because of the extent to
Copy link
Member

Choose a reason for hiding this comment

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

nit: Runtime deprecation? ;)

* used moving forward. Rather, developers should use one of the three new
* factory APIs: Buffer.from(), Buffer.allocUnsafe() or Buffer.alloc() based on
* their specific needs. There is no hard deprecation because of the extent to
* which the Buffer constructor is used in the ecosystem currently -- a hard
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)

const prefix = `(${process.release.name}:${process.pid}) `;

exports.setup = setupProcessWarnings;

var fd;
function acquireFd() {
if (!fd) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: fd === undefined

}
if (code && typeof code !== 'string')
throw new TypeError('`code` must be a String');
if (name && typeof name !== 'string')
Copy link
Member

Choose a reason for hiding this comment

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

nit: name !== undefined and code !== undefined

@@ -51,11 +51,18 @@ exports._deprecate = function(fn, msg) {
return fn;
}

if (code && typeof code !== 'string')
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)

@@ -44,6 +45,16 @@ void InitConfig(Local<Object> target,

if (config_preserve_symlinks)
READONLY_BOOLEAN_PROPERTY("preserveSymlinks");

if (config_warning_file != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: nullptr

@jasnell jasnell force-pushed the warning-additions branch 2 times, most recently from 5bf38a7 to 3b98c43 Compare December 4, 2016 22:19
@jasnell
Copy link
Member Author

jasnell commented Dec 4, 2016

@addaleax @Fishrock123 ... updated to address the nits. Updated the --redirect-warnings to append and renamed the emitWarning() name argument to type

@@ -706,14 +706,15 @@ console.log(process.env.test);
// => 1
```

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

@silverwind silverwind Dec 5, 2016

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.

Copy link
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?

Copy link
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.

Copy link
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

Copy link
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.

Copy link
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.

Copy link
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

Copy link
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.

Copy link
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.

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2016 via email


Type: Documentation-only

The `Buffer()` and `new Buffer()` constructors are considered to be deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Buffer() is not considered as a constructor

As an alternative, use of the following methods of constructing `Buffer` objects
is strongly recommended:

* `Buffer.alloc(size[, fill[, encoding]])` - Create a `Buffer` with
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing links to these methods will be helpful.


Type: Runtime

The `crypto.Credentials` class is deprecated. Please Uuse `tls.SecureContext`
Copy link
Contributor

Choose a reason for hiding this comment

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

use

Type: Runtime

The `constants` module has been deprecated. When requiring access to constants
relevant to specific Node.js built in modules, developers should instead refer
Copy link
Contributor

Choose a reason for hiding this comment

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

built-in


Type: Runtime

The `fs.read()` legacy String interface is deprecated. Use the Buffer API as
Copy link
Contributor

Choose a reason for hiding this comment

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

We explicitly ask the readers to refer documentation, it would be better if we had the link here.


Type: Documentation-only

The `require.extensions` API has been deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

property or API

@@ -1,8 +1,9 @@
'use strict';

// This module is soft-deprecated. Users should be directed towards using
// This module is deprecate in docs. Users should be directed towards using
Copy link
Contributor

Choose a reason for hiding this comment

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

doc-deprecated?

if (warning.code) {
output(`${prefix}[${warning.code}] ${toString.apply(warning)}`);
} else {
output(`${prefix}${toString.apply(warning)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a space after ${prefix}?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the ${prefix} includes a space already

if (config_warning_file != nullptr) {
target->DefineOwnProperty(env->context(),
OneByteString(env->isolate(), "warningFile"),
String::NewFromUtf8(env->isolate(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we split them into multiple variables?

fork(warnmod, {env: {NODE_REDIRECT_WARNINGS: warnpath}})
.on('exit', common.mustCall(() => {
fs.stat(warnpath, common.mustCall((err, stat) => {
assert(!err);
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.ifError

@silverwind
Copy link
Contributor

silverwind commented Dec 5, 2016

Switching to an options object would just add confusion, I think.

It's more of a general adivce to design APIs in a extendable way. I think that once a function takes 3 or more arguments, it gets to a point where the user has to lookup the arguments on less-frequently used functions, which could've been avoided if it would be designed with a options object from the start.

@Fishrock123
Copy link
Contributor

What do we do about things we have to un-deprecate? Just remove them from the doc? Or mark them as no longer deprecated?

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2016 via email

@jasnell jasnell force-pushed the warning-additions branch 2 times, most recently from d6799a0 to eea42cb Compare December 5, 2016 18:11
@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2016

@silverwind ... I don't disagree, but as I said, this API already provides the option of passing in an Error object as the first argument. Given that, I don't see significant value in providing an options object

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2016

@thefourtheye ... updated to address your feedback. I squashed and force pushed to keep things organized.

@jasnell jasnell force-pushed the warning-additions branch 2 times, most recently from 658588a to e5d79fd Compare December 5, 2016 18:24
@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2016

@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2016

@nodejs/ctc ... thoughts on this?

@trevnorris
Copy link
Contributor

@jasnell there a way to automatically track the next deprecation code? feel it could be error prone or frustrating to grep all the current ones to find the next.

@jasnell
Copy link
Member Author

jasnell commented Dec 9, 2016 via email

@trevnorris
Copy link
Contributor

@jasnell ah. missed deprecations.md b/c github refrained from loading it. okay, seems reasonable enough.

@sam-github
Copy link
Contributor

I backported 03e89b3 to 6.x, the other commits look semver-major to me, as they add deprecation code and change the output format. Do I misunderstand?

sam-github pushed a commit to sam-github/node that referenced this pull request Jul 24, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

PR-URL: nodejs#10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
sam-github pushed a commit that referenced this pull request Jul 24, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: #14418
PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Jeyanthinath pushed a commit to Jeyanthinath/node that referenced this pull request Jul 26, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: nodejs#14418
PR-URL: nodejs#10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this pull request Oct 10, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

PR-URL: nodejs#10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: #12677
PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
The --redirect-warnings command line argument allows process warnings
to be written to a specified file rather than printed to stderr.

Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable.

If the specified file cannot be opened or written to for any reason,
the argument is ignored and the warning is printed to stderr.

If the file already exists, it will be appended to.

Backport-PR-URL: #12677
PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
MylesBorins added a commit that referenced this pull request Nov 6, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
MylesBorins added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
@targos targos removed their assignment Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. deprecations Issues and PRs related to deprecations. lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.