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

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

Closed
jhnns opened this issue Mar 2, 2017 · 12 comments
Closed

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

jhnns opened this issue Mar 2, 2017 · 12 comments
Labels
question Issues that look for answers. util Issues and PRs related to the built-in util module.

Comments

@jhnns
Copy link
Contributor

jhnns commented Mar 2, 2017

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 9cd44bb).

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:

  • Either remove util.deprecate() from the documentation or remove the (node) prefix
  • In any case: The output of the warning can be improved. For example, we could add the last one or two lines of the stack trace by default to reveal which library is causing the deprecation warning. We could also add an URL to the documentation that lists all the available cli flags. I think, elm is doing an incredible good job of explaining errors/problems.
@mscdex mscdex added util Issues and PRs related to the built-in util module. question Issues that look for answers. labels Mar 2, 2017
@sam-github
Copy link
Contributor

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

@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
Copy link
Contributor Author

jhnns commented Mar 3, 2017

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
Copy link
Contributor

seishun commented Mar 3, 2017

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

jasnell commented Mar 3, 2017

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
Copy link
Contributor

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

jasnell commented Mar 3, 2017

@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
Copy link
Contributor

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

dougwilson commented Mar 3, 2017

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 #4782 (comment)

@jhnns
Copy link
Contributor Author

jhnns commented Mar 5, 2017

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.

@jhnns jhnns changed the title Clarify usage of util.deprecate() Should util.deprecate() be removed from the public API? Mar 5, 2017
@Fishrock123
Copy link
Contributor

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

@addaleax
Copy link
Member

Given that #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

8 participants