Add a process.emitWarning for deprecated
#202
Conversation
The Archiver.bulk() call has been deprecated for a while and various answers on SO and other sites still suggest it, so if there are no planned maintenance fixes and updates to that function, everyone (even those who do not read the docs) should be informed that they are using something that is deprecated.
lib/core.js
Outdated
| * @return {this} | ||
| */ | ||
| Archiver.prototype.bulk = function(mappings) { | ||
| console.log('Archiver.bulk() deprecated since 0.21.0'); |
There was a problem hiding this comment.
Better do console.warn (or console.error) it's safer to not pollute the stdout.
We also could leverage the new process.emitWarning? Only restriction is that it's node 6.0.0+ only.
There was a problem hiding this comment.
The console.warn function is an alias for console.error according to docs, so no difference in those. I didn't know about the process.emitWarning so I will probably switch it over to that and figure out how to gracefully fall back to console.warn when run on older versions. There also appears to be a DeprecationWarning for the emitting which can be useful.
There was a problem hiding this comment.
Yes I know for the alias that's why I specified (or console.error).
+1 about the fall back, you can just test process && process.emitWarning !== undefined (where process is in case it's beeing used in the navigator)
There was a problem hiding this comment.
Pushed 98fa973 to prefer process.emitWarning over console.warn.
The process.emitWarning call is much fancier than the plain logging call as the user can --throw-deprecation or --no-deprecation if he or she prefers to. As a fallback, we just console.warn to stderr.
sigv
changed the title
sigv
changed the title
deprecated
sigv
changed the title
deprecatedprocess.emitWarning for deprecated
lib/core.js
Outdated
| console.log('Archiver.bulk() deprecated since 0.21.0'); | ||
| if (typeof process !== 'undefined' && typeof process.emitWarning !== 'undefined') { | ||
| process.emitWarning('Archiver.bulk() deprecated since 0.21.0', 'DeprecationWarning'); | ||
| } else if (typeof console !== 'undefined' && typeof console.warn !== 'undefined') { |
There was a problem hiding this comment.
If you think checking for availability of console.warn is overkill, let me know and I'll drop this check.
| process.emitWarning('Archiver.bulk() deprecated since 0.21.0', 'DeprecationWarning'); | ||
| } else if (typeof console !== 'undefined' && typeof console.warn !== 'undefined') { | ||
| console.warn('Archiver.bulk() deprecated since 0.21.0'); | ||
| } |
There was a problem hiding this comment.
+1 test of console is overkill, also you might want to declare a variable for the error string to avoid repeating it.
Then 👍
The deprecation logging action should only take place once so that the client does not get spammed with the warnings. Additionally, the console is now always expected to be around.
| process.emitWarning('Archiver.bulk() deprecated since 0.21.0', 'DeprecationWarning'); | ||
| } else { | ||
| console.warn('Archiver.bulk() deprecated since 0.21.0'); | ||
| } |
There was a problem hiding this comment.
Oh sorry there's been a misunderstanding,
I was thinking into something like:
var warning = 'Archiver.bulk() deprecated since 0.21.0'
if (typeof process !== 'undefined' && typeof process.emitWarning !== 'undefined') {
process.emitWarning(warning, 'DeprecationWarning');
} else {
console.warn(warning);
}
There was a problem hiding this comment.
Okay, that can be done as well. Do you want me to leave the _loggedBulkDeprecation in there or do you want me to log on each call?
There was a problem hiding this comment.
Pushed 86b1123 which deals with the repeated string. If you want me to drop the _loggedBulkDeprecation, let me know.
There was a problem hiding this comment.
It's a nice improvement imo, let's see what @ctalkington thinks
Code maintainability and clean'ness and such features are included in this commit.
|
How long does it usually take for @ctalkington to look into proposed changes? |
|
He is really busy but no worries it'll eventually get merged ;) |
|
I expected something like having this merged next week or so if you were so busy. |
|
@sigv sorry this took so long to publish, i try to do things in bundles but I went ahead and cut a 1.2.0 release that includes this. |
|
I appreciate a deprecation warning as much as the next guy, but this is a bit too much...
|
|
When you @soyuka You want me to set it on the |
4 participants
The Archiver.bulk() call has been deprecated for a while and various answers on SO and other sites still suggest it, so if there are no planned maintenance fixes and updates to that function, everyone (even those who do not read the docs) should be informed that they are using something that is deprecated.