nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108.06k stars 29.83k forks source link

Should util.deprecate() be removed from the public API? #11642

Closed jhnns closed 7 years ago

jhnns commented 7 years ago

Reading from the docs, it is not clear whether util.deprecate() should be used by userland modules or not. There is the preamble that states:

The util module is primarily designed to support the needs of Node.js' own internal APIs. However, many of the utilities are useful for application and module developers as well

Reading this, I would guess that it is ok to use util.deprecate(). The output, however, looks like this:

(node:<strange number>) DeprecationWarning <message>

The (node) prefix is indicating that the deprecation warning is coming from node itself (introduced with https://github.com/nodejs/node/commit/9cd44bb2b683446531306bbcff8739fc3526d02c).

If util.deprecate() is meant for internal use only, I think it would be better to remove it from the official documentation because it can be misleading for both the library authors and library users.


Just as background: I was using this method for a breaking change in webpack's loader-utils. This module is used by almost every loader, so it is very likely that it appeared at least once in the console of every webpack user. I was using this method because I think it's good to have a unified way of handling deprecations, especially in combination with the command line flags provided by node. I just wanted to stick to the community default (at least as it appeared to me).

Judging from the feedback, however, many users didn't know where the message was coming from and what to do. And I think this is also due to the unfriendly appearance of the message. It is lacking the necessary information whether if it's a problem the developer must fix or the library author. You could use --trace-deprecation, but if you never heard of util.deprecate(), you don't know that this flag exists.


Having said this, this is what I would like to change:

sam-github commented 7 years ago

I think we should deprecate util.deprecate() (I know, a bit ciruclar) and then move it to lib/internal (where it can only be called by node) as soon as our deprecation policy allows.

This is an example of exposing an API "because we made it for ourselves and it might be useful to someone else", and that exposure making it hard to change later to work better for Node.

dougwilson commented 7 years ago

@jhnns FWIW I made https://github.com/dougwilson/nodejs-depd for use in the Express system and it seems to handle the issues you're bringing up, notably it's public API for module authors to use and it provides a location of the issue by default (and even honors the Node.js command line flags, to boot).

jhnns commented 7 years ago

I think we should deprecate util.deprecate()

You can do that, but I think the output should still be improved. I'm seeing deprecation warnings every now and then in my console, and before I knew about the command line flags I was always like: "Oh, what's that? Is that an error? Where is it coming from?". I'm not the kind of developer who is comfortable with ignoring mysterious deprecation warnings 😁

As a developer, I'm concerned about the origin: Who is responsible for fixing this? And unless you've used util.deprecate() before or you know all cli flags by heart ❤️, you have no clue how to locate that warning.

FWIW I made https://github.com/dougwilson/nodejs-depd for use in the Express system and it seems to handle the issues you're bringing up, notably it's public API for module authors to use and it provides a location of the issue by default (and even honors the Node.js command line flags, to boot).

Yeah, I think I've seen your module before, but at the time of writing I didn't remember it anymore 😁.

Personally, I would rather like to see this functionality in core because it's something every library has to deal with – and node's own deprecation warnings would be better. Additionally, the node user would benefit from a unified way of tracing deprecations/disabling deprecation warnings.

If you decide to deprecate this and remove it from the public API, I think it would be beneficial to link to depd as userland alternative and to promote the CLI and process flags. Every deprecation library should honor these.

seishun commented 7 years ago

I agree with deprecating util.deprecate(). I think it should be up to the module authors to decide how exactly the deprecation warning should look like, whether it should honor --no-deprecation, and so on.

Also, util.deprecate() being public means that any changes to its behavior would have to be semver-major. For example, let's say we decide to add information about flags to the deprecation message. That might break some module's tests.

jasnell commented 7 years ago

To explain the syntax just a bit...

(node:<strange number>) [<deprecation code>] DeprecationWarning: message

The <strange number> is the PID of the node process. If you have multiple node processes running this number is helpful in differentiating which emitted the warning.

The [<deprecation code>] is a new addition. All Node.js generated deprecations will have a static identifier code in the form DEP00NN that will identify the deprecation in question. These can be looked up the in the documentation for more context.

At this point in time, util.deprecate() is a thin wrapper around the process.emitWarning() API. It is possible for a user to emit their own deprecation warnings using process.emitWarning('mesage', 'DeprecationWarning'). The util.deprecate() adds some additional logic around it to automatically handle function and class deprecations so that the message is only emitted once. As such, it definitely has utility and I have seen userland modules take advantage of it.

I would be -1 at deprecating it at this point simply because it is being used effectively and there's really no cost to keeping it.

sam-github commented 7 years ago

@jhnns Sorry, you mentioned too many unrelated things in this single issue. Besides what I said earlier, I think your suggestion of including a line or two of context is a decent one. I suggest you bring that suggestion up in a seperate issue (or PR it).

@jasnell I think the OP is contesting whether this API can be used effectively outside of node, given its output format.

jasnell commented 7 years ago

@sam-github ... yes, I got that, and I have seen it used effectively. The addition of the code actually makes it significantly easier. For instance, one could do util.deprecate(fn, 'message', 'MYDEPCODE') and the output would be: (node:<pid>) [MYDEPCODE] DeprecationWarning: message. The `MYDEPCODE then becomes a differentiator for userland deprecations.

sam-github commented 7 years ago

Without a registry of userland deprecation numbers, how will they allocate deprecation codes? Since the deprecation is identified as a node deprecation, I predict github issues opened on node for user-land deprecations coming soon. IMO, building this into node stifles innovation in the community.

dougwilson commented 7 years ago

I think your suggestion of including a line or two of context is a decent one. I suggest you bring that suggestion up in a seperate issue (or PR it).

That was discussed and rejected previously in https://github.com/nodejs/node/pull/4782#issuecomment-189501013

jhnns commented 7 years ago

I think it should be up to the module authors to decide how exactly the deprecation warning should look like, whether it should honor --no-deprecation, and so on.

What benefits come with this flexibility? That some modules will just ignore --no-deprecation? I don't see how that is beneficial to the user. That's why I thought that this should be provided by core.

The is the PID of the node process. If you have multiple node processes running this number is helpful in differentiating which emitted the warning.

Since there is no explanation in the message, I would have to read node's source code to find out that this is the process id. Furthermore, it feels strange to me that the process id is more important than parts of the stack trace.

Now I understand the reasoning behind it, but it's sad that the default is optimized to be parsed by machines instead to be read by humans. Because I'd assume that many warnings will be read by humans. And unless you've heard of --trace-warnings or the warning API before, there is no clue how to deal with that message. I think it would already help to append something like Use the --trace-warnings command line flag to get a full stack trace of this warning.

Sorry, you mentioned too many unrelated things in this single issue. Besides what I said earlier, I think your suggestion of including a line or two of context is a decent one. I suggest you bring that suggestion up in a seperate issue (or PR it).

You're right. I've created a dedicated issue for that: #11703

This discussion should be about removing util.deprecate() from the public API.

Fishrock123 commented 7 years ago

I think we should have a publicly supported way for user-land runtime deprecations.

addaleax commented 7 years ago

Given that https://github.com/nodejs/node/pull/12631 was closed, I’m going to close this too as “util.deprecate() is here to stay”. If you think we need to have more discussion on this, please feel free to re-open.