GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — process: Add code to warnings, assign codes to deprecations by jasnell · Pull Request #10116 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Dec 4, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell jasnell added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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 involving general changes in the lib/ or src/ directories. labels Dec 4, 2016
@jasnell

jasnell commented Dec 4, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/5176/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax
addaleax reviewed Dec 4, 2016
View reviewed changes
Comment thread doc/api/cli.md Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Dec 4, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fishrock123 Fishrock123 Dec 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@jasnell jasnell Dec 4, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 \)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/process.md Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fishrock123 Fishrock123 Dec 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Dec 4, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Dec 4, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/buffer.js Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: Runtime deprecation? ;)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/buffer.js Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(ditto)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/process/warning.js Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: fd === undefined

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/process/warning.js Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/util.js Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(ditto)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/node_config.cc Outdated

@addaleax addaleax Dec 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: nullptr

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jasnell commented Dec 4, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

silverwind
silverwind reviewed Dec 5, 2016
View reviewed changes
Comment thread doc/api/process.md Outdated

@silverwind silverwind Dec 5, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sam-github sam-github Jan 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Jan 10, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Jan 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sam-github sam-github Jan 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

silverwind reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@silverwind silverwind Jan 11, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Jan 11, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sam-github sam-github Jan 11, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Jan 11, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell

jasnell commented Dec 5, 2016 via email

Copy link
Copy Markdown
Member Author
I disagree. The method already has the option of passing in a Error object as the first parameter. Switching to an options object would just add confusion, I think.
…
On Sun, Dec 4, 2016 at 4:44 PM silverwind ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/api/process.md <#10116 (review)>: > @@ -706,14 +706,15 @@ console.log(process.env.test); // => 1 ``` -## process.emitWarning(warning[, name][, ctor]) +## process.emitWarning(warning[, type[, code]][, ctor]) I think a options object is in order here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10116 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2ec-j0kNP2A0JnuBeKN3ejbShHNY-ks5rE15ugaJpZM4LDsqo> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thefourtheye
thefourtheye reviewed Dec 5, 2016
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

use

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

built-in

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/api/deprecations.md Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

property or API

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/constants.js Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

doc-deprecated?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/process/warning.js Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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}?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Dec 5, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/node_config.cc Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-process-redirect-warnings-env.js Outdated

@thefourtheye thefourtheye Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

assert.ifError

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@silverwind

silverwind commented Dec 5, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123

Fishrock123 commented Dec 5, 2016

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Dec 5, 2016 via email

Copy link
Copy Markdown
Member Author
I'd leave them in the doc and label them as undeprecated. The codes should increase monotonically so leaving them in the doc will avoid collisions
…
On Mon, Dec 5, 2016 at 7:10 AM Jeremiah Senkpiel ***@***.***> wrote: What do we do about things we have to un-deprecate? Just remove them from the doc? Or mark them as no longer deprecated? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eWrYaP8LINcM1uUgqt8HFsZHXYIaks5rFClXgaJpZM4LDsqo> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/5211/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell
jasnell force-pushed the warning-additions branch from e5d79fd to 3a19ece Compare December 5, 2016 19:57
@jasnell

jasnell commented Dec 6, 2016

Copy link
Copy Markdown
Member Author

@nodejs/ctc ... thoughts on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trevnorris

trevnorris commented Dec 9, 2016

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Dec 9, 2016 via email

Copy link
Copy Markdown
Member Author
There might be, but we need to make sure they stay static once assigned. Best bet would be to check the deprecations.md and assign the next highest, then make sure that doc always gets updated with a new deprecation
…
On Fri, Dec 9, 2016 at 2:11 PM Trevor Norris ***@***.***> wrote: @jasnell <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2efCdwqCpSxTV1JESmkYNkFF7MQHwks5rGdIEgaJpZM4LDsqo> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trevnorris

trevnorris commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

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

jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell
jasnell requested review from mscdex and ofrobots December 11, 2016 00:55
@jasnell

jasnell commented Jan 27, 2017

Copy link
Copy Markdown
Member Author

@targos updated! PTAL
CI: https://ci.nodejs.org/job/node-test-pull-request/6084/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos
targos approved these changes Jan 28, 2017
View reviewed changes
@targos targos removed ctc-agenda labels Jan 28, 2017
jasnell added 3 commits January 30, 2017 11:02
@jasnell
process: add optional code to warnings + type checking
81db4e8
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.
@jasnell
process: add --redirect-warnings command line argument
be11fb4
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.
@jasnell
lib: add static identifier codes for all deprecations
ab50285
Assigns a static identifier code to all runtime and documentation
only deprecations. The identifier code is included in the emitted
DeprecationWarning.

Also adds a deprecations.md to the API docs to provide a central
location where deprecation codes can be referenced and explained.
@jasnell
jasnell force-pushed the warning-additions branch from b08dcb2 to ab50285 Compare January 30, 2017 19:07
jasnell added a commit that referenced this pull request Jan 30, 2017
@jasnell
process: add optional code to warnings + type checking
5e1f32f
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.

PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
jasnell added a commit that referenced this pull request Jan 30, 2017
@jasnell
process: add --redirect-warnings command line argument
03e89b3
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: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
jasnell added a commit that referenced this pull request Jan 30, 2017
@jasnell
lib: add static identifier codes for all deprecations
5de3cf0
Assigns a static identifier code to all runtime and documentation
only deprecations. The identifier code is included in the emitted
DeprecationWarning.

Also adds a deprecations.md to the API docs to provide a central
location where deprecation codes can be referenced and explained.

PR-URL: #10116
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@jasnell

jasnell commented Jan 30, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Landed in 5e1f32f...5de3cf0

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell closed this Jan 30, 2017
@Trott Trott removed the ctc-review label Jan 31, 2017
@sam-github sam-github mentioned this pull request Feb 9, 2017
src: make copies of startup environment variables #11051
Closed
@sam-github

sam-github commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

@jasnell do you plan to backport this? Its a useful feature, and it appears to not be necessarily semver-major:

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.

Its blocking later PRs that touch nearby code even though they don't depend on this feature, like #11051, and its follow ons.

I can backport if you don't have time, but would like it in 7.x.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Feb 9, 2017

Copy link
Copy Markdown
Member Author

I backported it to 4.x today and opened a PR. It should be fairly trivial to backport to 7.x and 6.x. I wouldn't be able to get to it until next week so if you'd like to go for it, be my guest.

btw... I would also include the new NODE_NO_WARNINGS environment variable if that has not already been backported

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ChALkeR ChALkeR mentioned this pull request Feb 12, 2017
meta: ctc-agenda label is misused according to the Project Governance #11325
Closed
@sam-github sam-github mentioned this pull request Feb 13, 2017
7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345
Closed
4 tasks
@jasnell jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed
jasnell added a commit to jasnell/node that referenced this pull request May 29, 2017
@jasnell
2017-05-30, Version 8.0.0 (Current)
c596751
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](nodejs@4a7233c178)]
    [nodejs#12892](nodejs#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](nodejs@d2d32ea5a2)]
    [nodejs#11968](nodejs#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](nodejs@7eb1b4658e)]
    [nodejs#12141](nodejs#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](nodejs@beca3244e2)]
    [nodejs#10236](nodejs#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](nodejs@97a77288ce)]
    [nodejs#12348](nodejs#12348),
    [[`d75fdd96aa`](nodejs@d75fdd96aa)]
    [nodejs#10423](nodejs#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](nodejs@627ecee9ed)]
    [nodejs#10653](nodejs#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](nodejs@f18e08d820)]
    [nodejs#9744](nodejs#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](nodejs@3c3b36af0f)]
    [nodejs#12936](nodejs#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](nodejs@60d1aac8d2)]
    [nodejs#12784](nodejs#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](nodejs@84dabe8373)]
    [nodejs#12489](nodejs#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](nodejs@7a55e34ef4)]
    [nodejs#10467](nodejs#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](nodejs@3c2a9361ff)]
    [nodejs#9683](nodejs#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](nodejs@90403dd1d0)]
    [nodejs#11567](nodejs#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](nodejs@d3480776c7)]
    [nodejs#11259](nodejs#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](nodejs@fb71ba4921)]
    [nodejs#11355](nodejs#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](nodejs@3e6f1032a4)]
    [nodejs#10805](nodejs#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](nodejs@5de3cf099c)]
    [nodejs#10116](nodejs#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](nodejs@84a23391f6)]
    [nodejs#12113](nodejs#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](nodejs@56e881d0b0)]
    [nodejs#11975](nodejs#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](nodejs@03e89b3ff2)]
    [nodejs#10116](nodejs#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](nodejs@dd20e68b0f)]
    [nodejs#12725](nodejs#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](nodejs@3f27f02da0)]
    [nodejs#11599](nodejs#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (nodejs@ec7cbaf266)]
    [nodejs#12995](nodejs#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](nodejs@a16b570f8c)]
    [nodejs#11968](nodejs#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](nodejs@010f864426)]
    [nodejs#12949](nodejs#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](nodejs@a5f91ab230)]
    [nodejs#11689](nodejs#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](nodejs@8a7db9d4b5)]
    [nodejs#12087](nodejs#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](nodejs@b6e1d22fa6)]
    [nodejs#12925](nodejs#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](nodejs@07c7f198db)]
    [nodejs#12828](nodejs#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](nodejs@348cc80a3c)]
    [nodejs#5923](nodejs#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](nodejs@a2ae08999b)]
    [nodejs#11349](nodejs#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)]
    [nodejs#11447](nodejs#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](nodejs@d080ead0f9)]
    [nodejs#12710](nodejs#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](nodejs@5bfd13b81e)]
    [nodejs#9726](nodejs#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](nodejs@455e6f1dd8)]
    [nodejs#11708](nodejs#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](nodejs@aab0d202f8)]
    [nodejs#11624](nodejs#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](nodejs@99da8e8e02)]
    [nodejs#12442](nodejs#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](nodejs@91383e47fd)]
    [nodejs#12001](nodejs#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](nodejs@b514bd231e)]
    [nodejs#11391](nodejs#11391).
This was referenced Jul 21, 2017
src: redirect-warnings to file, not path #13120
Closed
(v6.x backport) process: add --redirect-warnings command line argument #14418
Merged
@sam-github

sam-github commented Jul 21, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github sam-github mentioned this pull request Jul 25, 2017
(v6.x backport) process: add --redirect-warnings command line argument #14480
Closed
4 tasks
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
v6.12.0 proposal #16263
Merged
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
v4.8.6 proposal #16500
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments
@Trott Trott Trott left review comments
@addaleax addaleax addaleax left review comments
@ofrobots ofrobots Awaiting requested review from ofrobots
@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis
@trevnorris trevnorris Awaiting requested review from trevnorris
@MylesBorins MylesBorins Awaiting requested review from MylesBorins
@srl295 srl295 Awaiting requested review from srl295
+10 more reviewers
@sam-github sam-github sam-github approved these changes
@mscdex mscdex mscdex left review comments
@silverwind silverwind silverwind left review comments
@rvagg rvagg rvagg left review comments
@evanlucas evanlucas evanlucas left review comments
@thefourtheye thefourtheye thefourtheye left review comments
@Fishrock123 Fishrock123 Fishrock123 left review comments
@indutny indutny indutny approved these changes
@targos targos targos approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@Fishrock123 Fishrock123

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 involving general changes in the lib/ or src/ directories. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

18 participants

@jasnell @silverwind @Fishrock123 @trevnorris @ChALkeR @Trott @rvagg @joshgav @sam-github @mscdex @indutny @evanlucas @thefourtheye @addaleax @targos @mhdawson @tniessen @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.