process: add 'warning' event

[geek]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)? need to fix eslint rule for allowing template strings
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

process

Description of change

This is a modified version of the pull request from @jasnell at #4782

Instead of creating custom warning types, this PR uses the Error object and adds a name property that represents the type of warning thats emitted. The emitWarning does't exist in this PR, it didn't seem necessary. Also, I didn't add the SecurityWarning type, as its not used.

/cc @rvagg see what you think

[jasnell]

In general, this looks good... it means a bit more work for someone adding a new warning but that's perfectly fine. Those changes look fine. I am a bit concerned that someone would need to make sure to set the .name consistently (ensuring consistency was part of the justification for the internal/warning utility)

The emitWarning() is something that reviewers on the PR have indicated they would like and I've come to view it as a useful addition. I'd like to see that remain.

[rvagg]

I share similar concerns with @jasnell, perhaps an internalUtil.warning() would be best to avoid the repetition that's already going on here and avoid the future potential for forgetting to do it in just the right way.

[estliberitas]

Shouldn't it be const message?

[cjihrig]

I think it would be useful to have either emitWarning() or create a Warning type that extends Error. I don't think we need both, but we should have one. My personal preference would be to extend Error, as it would allow users to extend Warning in a more standard way.

[dougwilson]

The entire Express.js code base deals with deprecations in a similar way, hooking into --*-deprecation process flags and emitting deprecations as a deprecation event on the process object to allow hooks into logging. The objects emitted are also inherited from Error, to provide not only stack traces, but integration into logging tools like Winston that already know how to serialize these types of objects. Providing a fallback to print to console with this is a little harder than Express.js has to deal with, as since Express.js is emitting the events, it can just choose to print if there are no listeners.

Express.js further provides mechanisms to suppress deprecations on a module-by-module basis, rather that only all or nothing (though you can do all-or-nothing, either by using the --no-deprecation command switch, or by telling it to ignore all modules).

Anyway, I just happened to stumble on this issue and just wanted to provide my 2c on what Express.js has been doing for almost 2 years now.

My main concern with this pull request is that it adds a new --*-warning command line set, in addition to the --*-deprecation command line set, yet both of those things end up being emitted through the same event warning. Why not having warnings emitting under warning and deprecations emitted under deprecation? They are pretty difference classes of information, as even indicated by the comment line arguments, but also in the necessity to fix the issue from a developer point-of-view.

[jasnell]

The warnings really ought to be emitted on the next tick

[jasnell]

@cjihrig ... it's worth noting that the Warning object is only used internally and is not exposed as part of the public facing API. All end users will see is the process.emitWarning() API. Internally, it's used simply to ensure that the warning itself is constructed consistently. I'm looking at refactoring this just a bit based on the feedback, however

[jasnell]

Comparing this with my original #4782 ... I think I still favor mine albeit with a number of the changes suggested here. I will be pushing an update shortly with those changes. The approach I take ensure that there will be more consistency in the way the warnings are structured and emitted and hides some of that complexity from the code where the warning is actually being emitted. process.emitWarning was added specifically due to user feedback on the PR.

[jasnell]

I have pushed some updates to #4782 that incorporate a number of these changes. PTAL

[dougwilson]

@jasnell, any thoughts on the concern I brought up?

[geek]

Updated with feedback. This is very much the same as the PR from @jasnell, only without the custom warning wrapper. I also changed the signature of emitWarning to be ({ Error | String}[, String]) as the object options argument wasn't used.

[jasnell]

@geek ... btw, thanks for this PR ;-) .. whichever one lands ultimately will be good I think

@dougwilson ... I'd rather not have separate deprecate and warning events as they really are quite similar. This new mechanism should not interfere with what express is doing. That said, If others feel that there is value in separating those, however, it would be trivial to do so and I'd be fine with that. Outside of express, are you aware of any other modules that use a 'deprecation' event?

[jasnell]

@dougwilson ... just looking at depd again... and I think you've swayed me. This is an opportunity to get some good alignment between core and express. It would be fairly trivial to implement this and the inclusion of the namespace parameter is a good idea.

Could depd be updated to make use of the new process.emitWarning() API if I added the ability to specify the namespace (or even without that, depd can create the DeprecationError and simply pass it to emitWarning) ? You'll have to look at #4782 to see the emitWarning as @geek pulled it from his version here.

[jasnell]

@dougwilson ... one thing on this tho... there would be a default handler registered for the 'deprecation' event in order to do the default printing to stderr. That would impact depd's current check (the listenercount check would return 1). I don't think it's a good idea to include that check in the default handling in core because I don't want warnings to get hidden from the user if some sub-dependency happens to register a listener and the user has no idea it's happening.

[dougwilson]

Hi @jasnell, I would really want to tie that module into this mechanism, which is why I looked at this PR when I saw it's title when browsing Node.js issues :) Having a common mechanism is awesome in the long run, and really depd could just be a user-land interface onto that code interface, IDK.

I would say probably an aspect of depd being able to use the emitWarning would be as long as we can provide a stack trace, as the stack trace that module generates has no relation to where the deprecation message was printed from, for various reasons of user diagnosis. It looks like the pull request already provides that mechanism :)

there would be a default handler registered for the 'deprecation' event in order to do the default printing to stderr. That would impact depd's current check (the listenercount check would return 1).

That is actually OK, because it means that all existing users who are just getting the default message printing will automatically get formatting from Node.js core. There isn't a default listener for deprecation (yet?) in the other pull request, so I can't say much more than that. The non-trace single-line messages emitted from depd do include a single line of the stack, because it is extremely useful to users, but this is missing from the mechanisms being added here to Node.js core (but of course, the way the stack is calculated by the depd module is very different than what is being done here, so the first line depd comes up with is exactly the point the user wants to change, so is useful being printed by itself).

[dougwilson]

And in case the description of how depd shows messages by default was confusing, perhaps an example of this in action may help. If you install the latest Express and save the following as app.js:

var express = require('express');
var app = express();

app.get('/', function (req, res) {
    res.send('hi!');
});

app.del('/', function (req, res) {
    res.send('bye!');
});

app.listen(4000);

When you invoke that file with node app.js, the following is the console output, by default:

express deprecated app.del: Use app.delete instead app.js:8:5

It would be nice if this default-ness would be preserved in some way, but perhaps that is a different issue with the implemented deprecation listener in the pull request, post-landing a fix. From what I can tell, if the current warning listener is basically what the default deprecation handler would be, the result of that application would end up being:

DeprecationError: app.del: Use app.delete instead

Which a use can kind of figure out where they went wrong, assuming that they have a very tiny codebase, and are not using a ton of npm modules.

I have found with the existing warnings that Node.js provides (and this pull request would continue to provide) is extremely confusing to users, and they end up simply trying to use grep on their code base to figure out where it comes from (many users are very unaware of the existing --trace-deprecation flag, or even know how to get a flag to run on their cloud hosting provider). This has resulted in several issues created in the mysql module (mysqljs/mysql#1203 and mysqljs/mysql#1154 are recent examples) because since that module supports many Node.js versions, the source contains a deprecated call, though it is never called on the versions of Node.js that deprecated it, but people find it using grep and file bugs over and over out of confusion from a lack of a pointer to the code location that is causing the warning.

I hope this helps think about the feature and how it can best help the Node.js users.

[jasnell]

Yep, once we're a bit more settled on the general mechanism, I'd like to work on the default formatting. I'm looking at possible adding the namespace property in the process.emitWarning. But, given that those pieces are in #4782 and not this PR, let's move further discussion on that over there so the discussion here can be focused on the specifics of @geek's counter-proposal. I feel like we accidentally took over and bifurcated the discussion (sorry @geek!).

[dougwilson]

But, given that those pieces are in #4782 and not this PR, let's move further discussion on that over there so the discussion here can be focused on the specifics of @geek's counter-proposal. I feel like we accidentally took over and bifurcated the discussion (sorry @geek!).

Yes, sorry. I originally made my comment in the other PR, but then it had looked like (at least as an outsider), that the work moved over here, so I had moved my concern here as well, I will continue discussion over there :)

[Fishrock123]

James' version landed in c6656db, thanks for the input!