-
Notifications
You must be signed in to change notification settings - Fork 30k
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
util: util.deprecate
improvement
#1892
Conversation
f7e2ba5
to
08e57b4
Compare
Regarding |
@silverwind Oh, shall I change |
Yeah, probably best of you'd change that. |
function deprecated() { | ||
var location = (new Error()).stack.match(/.*?\n.*\n.*?\((.*?:\d+):/)[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to match the correct line of the call. Here's a random test I put into an app of mine:
Stack:
Error
at startListeners (/Users/silverwind/git/droppy/server/server.js:121:18)
at /Users/silverwind/git/droppy/server/server.js:77:45
at /Users/silverwind/git/droppy/node_modules/async/lib/async.js:695:13
at iterate (/Users/silverwind/git/droppy/node_modules/async/lib/async.js:256:13)
at doNTCallback0 (node.js:408:9)
at process._tickCallback (node.js:337:13)
Output of your regex:
/Users/silverwind/git/droppy/node_modules/async/lib/async.js:256
You should try matching the first line here (server.js:121).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this with a real deprecate yet, so maybe I'm wrong 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind You are correct, I am rebasing now. I ll fix it in that. I didn't know that we will get lines like
at /Users/silverwind/git/droppy/server/server.js:77:45
this in the trace. Probably, I ll split the stack, take the second line and then parse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind Do you have the change which landed today? This should ideally have an entry for deprecated
. Are you doing only new Error().stack
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line above was produced by just
console.log(stack.match(/.*?\n.*\n.*?\((.*?:\d+):/)[1]);
I think the lines without parens are anonymous functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind For anonymous functions, I am getting Object.<anonymous>
. The lines without parens are for the calls directly from the modules, I think.
Also, apparently your branch is outdated and the PR needs a rebase already. |
If we actually want this it makes sense to use internal wrapper that prepends |
@vkurchatkin not a huge fan of a wrapper. Maybe there's a way to detect an internal call to |
@silverwind @vkurchatkin Why don't we accept the prefix as a parameter? |
@silverwind why not? just use @thefourtheye concatenating strings is something that users can do on their own |
@vkurchatkin guess I could live with something like |
You guys mean, |
Add a wrapper function in |
08e57b4
to
0570e40
Compare
@silverwind @vkurchatkin Thanks for the suggestions :-) I updated the PR. Please review. |
0570e40
to
2aa754e
Compare
|
||
|
||
exports.pump = exports.deprecate(function(readStream, writeStream, callback) { | ||
exports.pump = internalUtil.deprecate(function(readStream, writeStream, cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change this because of the line length restriction
2aa754e
to
876ff26
Compare
@silverwind I think I addressed all the comments and included a test with parenthesis in the file name. Please check now. |
@@ -3,4 +3,4 @@ | |||
const util = require('internal/util'); | |||
|
|||
module.exports = require('internal/smalloc'); | |||
util.printDeprecationMessage('smalloc is deprecated.'); | |||
util.deprecate('smalloc is deprecated. Use typed arrays.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will work here. The idea is to print a deprecation message when this line is executed.
printDeprecationMessage
should stay but we have a problem with the --trace-deprecation
flag. The actual caller location is where require('smalloc')
is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@targos Oh, you are correct. printDeprecationMessage
should stay. But, in case of require
, we can print only once. So, --trace-deprecation
cannot do much here, right?
876ff26
to
6d4a717
Compare
Is there a test yet to assert file and line numbers are correct? I can see this getting broken easily when the call stack to deprecate ever changes, so I think such a test is necessary. |
6d4a717
to
a24698a
Compare
@silverwind Oh yeah. You are correct. I updated one of the tests to include the checks. Please check now. |
a24698a
to
5e165c5
Compare
assert.equal(stack.pop(), '\'This is deprecated\''); | ||
console.log('trace ok'); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, But one newline is enough here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind Only line 114 is left empty, right? I thought that it would improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about the final line of this file. git apply also complains: new blank line at EOF
Gave it a try, seems the file and line number isn't always correct: $ >t.js
require("smalloc")
^C
$ ./iojs t.js
(node) smalloc is deprecated. Use typed arrays.
Used at: node.js:937 Strangely, Also, I'd prefer a single-line style. Maybe just cut out the path and use only the filename to keep it short:
or postfix, I have no preference:
|
Ah, that is a corner case where we skip the |
@silverwind Why don't we change the newly landed |
That could work too, just seems a bit confusing at first glance that |
@brendanashworth Thanks for the heads up :-) @silverwind I renamed it to |
@@ -10,7 +18,7 @@ exports.printDeprecationMessage = function(msg, warned) { | |||
if (process.throwDeprecation) | |||
throw new Error(msg); | |||
else if (process.traceDeprecation) | |||
console.trace(msg); | |||
console.trace(msg.replace(new RegExp(`^\\(${prefix}\\) `), '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand why this double escaping is needed. Wouldn't a literal like /^\(${prefix}\) /
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind Will the string substitution work in RegEx literals also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course:
> '(node) something'.replace(/\(node\) /,'')
'something'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not substitution right? I stored node
in a variable. How can that be substituted in the RegEx literal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Template string substitution won't work inside a regex literal, so this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind Actually, the ES6 proposal had re
Quasi literal function though :-)
Sorry, but still not totally satisfied yet. Here's what I suggest to fix the remaining issues:
|
Also, I think you should squash into a single commit now. I can't apply this PR on top of master because of previous patches that don't apply anymore. |
9825c35
to
416a134
Compare
@silverwind I updated the PR with the review comments. Please review now. |
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.
Sorry for the delay. LGTM |
Would it make more sense to have a exports.deprecate = function(oldValue, newValue) {
// using whatever appropriate internal util function instead of `console.log` of course
console.log('%s is deprecated. Use %s instead.', oldValue, newValue);
}; |
I see some use in something like that (primarily the part of forcing the maintainer to point to an alternative). But remember this is public API which would force a semver-major. |
Well, there could be a conditional to check if |
What if the users want to give a custom message? Let's say "Feature X is deprecated and removed in version 1.0.0. Use Y instead" |
I'm feeling more inclined to leave it as is with a simple string for flexibility. |
@silverwind What if we use @mscdex's suggestion for the internal deprecations? |
I'd decide against it. We can just make sure the future deprecation messages follow this scheme. No hand-holding needed there imho :) |
I'll most likely land this tomorrow, unless further objections are made. |
OSM :-) |
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]>
Landed in 9cd44bb |
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]>
Follow up of #1883.
Changes included in this PR are
Making the deprecation messages consistent. The messages will be in
the following format
If there is no alternative for
x
, then theUse y instead.
partwill not be there in the message.
All the internal deprecation messages are printed with the prefix
(node)
, except when the--trace-deprecation
flag is set.