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

Improve util.deprecate #1883

Closed
silverwind opened this issue Jun 3, 2015 · 17 comments
Closed

Improve util.deprecate #1883

silverwind opened this issue Jun 3, 2015 · 17 comments
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.

Comments

@silverwind
Copy link
Contributor

As discussed in IRC recently with @Fishrock123 it would be nice if the deprecation messages contain a bit more info. Particulary I'd like to see

  • filename:linenumber where it happens in userland code. I think this could be parsed off the stack through new Error().stack or similar, suggestions welcome.
  • A prefix like (node) to identify where the messages come from.
  • An flag like -print-all-deprecations to allow more than one print per deprecation to track down all places where it's happening.
@silverwind silverwind added the util Issues and PRs related to the built-in util module. label Jun 3, 2015
@thefourtheye
Copy link
Contributor

Why do we need the prefix? Can you include an example where it would be useful?

@silverwind
Copy link
Contributor Author

Look at line 6 of https://gist.github.com/silverwind/a94589df8e3aae1907e1, do you think an outsider would know what to do based on that message, especially when they happen during something like npm install.

@thefourtheye
Copy link
Contributor

Oh okay. You mean (request) ...? That would be good. About the line number and file name, what if the function is called more than once? Whenever user fixes one place he will get deprecation message for other places. Will that be okay?

@silverwind
Copy link
Contributor Author

No, I mean a prefix that clearly identifies that the line was logged by node. For multiple prints: I want to keep the functionality to only prince once per function call, of course. Multiple messages should be opt-in through the flag.

@Fishrock123
Copy link
Contributor

👍

@thefourtheye
Copy link
Contributor

@silverwind What if other libraries start using util.deprecate? We cannot log (node) in that case, right?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 3, 2015

Can't the existing --trace-deprecation flag be used to accomplish your first item? The second item seems a bit unnecessary. +1 to the third item.

@silverwind
Copy link
Contributor Author

@thefourtheye I think the intention is to move to an internal module eventually, @vkurchatkin had it moved in a recent PR if I recall correctly.

@silverwind
Copy link
Contributor Author

@cjihrig +1 on incorporating multiple prints into --trace-deprecation

@vkurchatkin
Copy link
Contributor

@silverwind no, it is just a helper function. util.deprecate is untouched.

filename:linenumber where it happens in userland code. I think this could be parsed off the stack through new Error().stack or similar, suggestions welcome.

This is already available with --trace-deprecation

A prefix like (node) to identify where the messages come from.

This could be just a part of the message

An flag like -print-all-deprecations to allow more than one print per deprecation to track down all places where it's happening.

+0

@silverwind
Copy link
Contributor Author

Right, I didn't realize we were exposing this knowingly in the public API, so it's probably best to include the (node) prefix in the messages itself.

@dougwilson
Copy link
Member

People who want a public API should probably just use the depd module or similar.

@Fishrock123
Copy link
Contributor

Right, but that doesn't really defer much from the fact that this already is public API.

The goal of what I was originally talking about was to better highlight spots where user code uses deprecated core APIs. However, I had forgotten about --trace-deprecation, which I think does that.

@silverwind
Copy link
Contributor Author

@dougwilson we gotta live with it being public now, or go through a tedious deprecation process of questionable benefit.

@Fishrock123 I didn't know about it either, but I think the default message should at least include the prefix and line number. The case of deprecation messages during npm install is a good example. These would be just confusing without context.

@thefourtheye
Copy link
Contributor

@silverwind The PR has the prefix change already. As we anyway extract the file name and line number from the stack trace, shall I just print them in normal deprecation warning as well?

@thefourtheye
Copy link
Contributor

I included a commit in the PR which will be actually printing the location where the deprecated item is used.

@brendanashworth brendanashworth added the feature request Issues that request new features to be added to Node.js. label Jun 24, 2015
silverwind pushed a commit that referenced this issue Jul 3, 2015
Changes included in this commit are

   1. Making the deprecation messages consistent. The messages will be in
      the following format

           x is deprecated. Use y instead.

      If there is no alternative for `x`, then the ` Use y instead.` part
      will not be there in the message.

   2. All the internal deprecation messages are printed with the prefix
      `(node) `, except when the `--trace-deprecation` flag is set.

Fixes: #1883
PR-URL: #1892
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor Author

Fixed by 9cd44bb with what was reasonable to change.

mscdex pushed a commit to mscdex/io.js that referenced this issue Jul 9, 2015
Changes included in this commit are

   1. Making the deprecation messages consistent. The messages will be in
      the following format

           x is deprecated. Use y instead.

      If there is no alternative for `x`, then the ` Use y instead.` part
      will not be there in the message.

   2. All the internal deprecation messages are printed with the prefix
      `(node) `, except when the `--trace-deprecation` flag is set.

Fixes: nodejs#1883
PR-URL: nodejs#1892
Reviewed-By: Roman Reiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

7 participants