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.

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

Jump to bottom

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

Closed
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 all commits
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
1 change: 1 addition & 0 deletions doc/api/_toc.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* [Console](console.html)
* [Crypto](crypto.html)
* [Debugger](debugger.html)
* [Deprecated APIs](deprecations.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe use the same word in the file name and the section name? I.e. deprecated.md/html or even deprecated-apis.md/html?

* [DNS](dns.html)
* [Domain](domain.html)
* [Errors](errors.html)
Expand Down
21 changes: 21 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ added: v6.0.0

Print stack traces for process warnings (including deprecations).

### `--redirect-warnings=file`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not terribly convinced this is useful enough to justify.

  1. It can't be triggered by env var, so its hard to configure in the cloud, or as a local user policy for people who just don't care and don't want to see the warnings
  2. warnings are emitted as events on process, specifically so this kindof handling can be implemented by the user, so why are we doing it for them?
  3. why just warnings? what about an option to redirect stack traces on uncaught errors or exceptions?
  4. why not just redirect stderr to a file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, docs don't say whether file is created or not, or whether it is appended to, or replaces the file.

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 idea for this came up during the collaborator summit and was specifically around finding a more intelligent way to handle deprecation warnings that are emitted when using tools like npm. When doing installs, it still may be necessary to capture stderr output as normal, but allow warnings to be deferred, captured, and processed in other ways.

This can be configured by environment variable. The envvar is included in this PR.

The file is created if it does not already exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missed the env var version, with that I'm only -0.

I was present in that conversation, and I don't think this addresses the experience of node users.

When doing npm installs, the only code that runs is npm's, so its not clear to me why we would want to silence the deprecation warnings. npm should just fix any of their deps to not cause these warnings, they are responsible citizens, they do that.

When using modules installed by npm, then I guess we don't care, usually, and can't do anything about them as a consumer of the module (other than report bugs). But, this redirect doesn't distringuish between deprecation warnings from our dependencies, and deprecations from out code itself, and node can't distinguish, it doesn't really know.

I think in the future it will look like an ad-hoc hack to deal with a more general problem, and we'll have to keep it around for a long time until we deprecate it.

That said, I am not prepared to argue more strongly against it. If noone else chimes in to object, its OK by me.

<!-- YAML
added: REPLACEME
-->

Write process warnings to the given file instead of printing to stderr. The
Copy link
Contributor

Choose a reason for hiding this comment

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

What can you pass to this? Could you pass a fd number and theoretically print these to "stdio 3" (even though that isn't default functionality, it could conceivably be made workable)?

Copy link
Member Author

Choose a reason for hiding this comment

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

A filepath only (absolute or relative). We could theoretically allow for an fd number but not sure if making it more complicated is worth it.... not out of the question tho.

file will be created if it does not exist, and will be appended to if it does.
If an error occurs while attempting to write the warning to the file, the
warning will be written to stderr instead.

### `--trace-sync-io`
<!-- YAML
added: v2.1.0
Expand Down Expand Up @@ -395,6 +405,17 @@ Note: Be aware that unless the child environment is explicitly set, this
evironment variable will be inherited by any child processes, and if they use
OpenSSL, it may cause them to trust the same CAs as node.

### `NODE_REDIRECT_WARNINGS=file`
<!-- YAML
added: REPLACEME
-->

When set, process warnings will be emitted to the given file instead of
printing to stderr. The file will be created if it does not exist, and will be
appended to if it does. If an error occurs while attempting to write the
warning to the file, the warning will be written to stderr instead. This is
equivalent to using the `--redirect-warnings=file` command-line flag.

[emit_warning]: process.html#process_process_emitwarning_warning_name_ctor
[Buffer]: buffer.html#buffer_buffer
[debugger]: debugger.html
Expand Down
Loading