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

Make warnings more beginner-friendly #11703

Closed
jhnns opened this issue Mar 5, 2017 · 11 comments
Closed

Make warnings more beginner-friendly #11703

jhnns opened this issue Mar 5, 2017 · 11 comments
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js.

Comments

@jhnns
Copy link
Contributor

jhnns commented Mar 5, 2017

This issue came up while discussing #11642

The problem with the warnings emitted by process.emitWarning is that they provide little information about the context or how to get more information. Unless the user knows about the cli flags like --trace-warnings or the warning API, there is no clue on how to proceed. A simple solution would be to append a short explanation like Use the --trace-warnings command line flag to get a full stack trace of this warning. if the flag is not set.

@mscdex mscdex added discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. labels Mar 5, 2017
@jhnns
Copy link
Contributor Author

jhnns commented Mar 5, 2017

It's also surprising that the warnings are prefixed with (${process.release.name}:${process.pid}). This makes sense when the output of multiple processes is piped into one destination, but it can be confusing in development mode, when the output is read by a human. To me, for instance, it was not clear that the number after : was the process id. I confused it with a line number. And you can only know that by guessing or reading node's source code 😁

@jhnns
Copy link
Contributor Author

jhnns commented Mar 5, 2017

Related discussion: #9516

@joyeecheung
Copy link
Member

About the format for deprecations: I think adding a URL to anchors in https://github.com/nodejs/node/blob/master/doc/api/deprecations.md (or on the website once they get rendered) could be more helpful(if stderr is TTY, maybe).

The explanation of the format can be added to that document too (along with how to --trace-deprecation and stuff).

@sam-github
Copy link
Contributor

wrt. the idea of adding anchor URLs to node's deprecations page, note that the deprecation API is deliberately exported for use by thirdparty modules, so changes to make it work better for node users can't be too node-specific.

@seishun
Copy link
Contributor

seishun commented Mar 11, 2017

@sam-github Which I think is a reason not to export the deprecation API.

Otherwise providing a URL in the deprecation message is a great idea. Listing all the deprecation-related flags would take up too much space.

@jhnns
Copy link
Contributor Author

jhnns commented Mar 13, 2017

Otherwise providing a URL in the deprecation message is a great idea

👍

Listing all the deprecation-related flags would take up too much space.

Seems reasonable. I'm just asking for a hint on how to proceed.

@sam-github
Copy link
Contributor

@jhnns the way to proceed is to PR an improvement to util.deprecate that includes useful information. People will have lots of comments, that's a good thing.

It may be that the changes are too large, and aren't backwards compatible. That's OK, if there is agreement that the messaging is much improved, then you'll just have to fork the util.deprecate API, make one in lib/internal/... to be used only by Node, and the external API can be left there, unchanged.

See: #12631 (comment)

Sorry for all the cross-linking, but the conversation spawns three threads ATM.

@jasnell
Copy link
Member

jasnell commented Apr 26, 2017

One possibility here is to add a new option to process.emitWarning() for additional detail. This can be added to the warning object emitted by process.on('warning') using a detail property and can be included in the default stderr output for all warnings. This would cover deprecations and warnings of all sorts.

@jasnell
Copy link
Member

jasnell commented Apr 26, 2017

Also note that the process.on('warning') API can easily be extended by third parties to provide additional capabilities. For instance, a module can easily attach a handler for DeprecationWarning that outputs the additional URL link.

@Trott
Copy link
Member

Trott commented Aug 4, 2017

Should this remain open? (I can't tell.)

@bnoordhuis
Copy link
Member

Another two months passed with no activity. I'll close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

8 participants