-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Remove deprecated Buffer.write(...) #5048
Conversation
@@ -561,14 +557,6 @@ Buffer.prototype.write = function(string, offset, length, encoding) { | |||
encoding = length; | |||
length = undefined; | |||
} | |||
|
|||
// XXX legacy write(string, encoding, offset, length) - remove in v0.13 | |||
} else { |
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.
Perhaps instead of flat out removing this branch, we should keep it and throw instead when isFinite(offset)
is false?
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.
Buffer.fill(val, start, end)
just coerces start
and end
to integers. This does the same with offset
. Any calls that still use the old arguments (string, encoding, offset, length)
will fail with an invalid encoding exception.
I can make it throw new Error("Buffer.write(string, encoding, offset, length) is no longer supported")
-- do you prefer that?
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.
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.
@dcposch Will (string, encoding, offset)
fail?
I'd prefer an explicit throw, at least for now.
I'd be fine with removing it... but let's see what @nodejs/ctc says.. |
@trevnorris this was yours, back in f489649, thoughts on whether it might even be used anywhere? Personally I'd not be in such a hurry to remove things just because they can be, particularly when they don't incur much cost. But whatever. |
@rvagg This legacy signature isn't even documented, and hasn't been since v0.10 IIRC. Plus it's had the deprecation warning in there for a while. I'd just like it removed b/c the argument parsing is ridiculous enough (takes up the first half of a 77 line function). But don't think it costs much performance wise leaving it there. So +1 for removing it, but also won't be upset if it remains. |
cc @ChALkeR for user land usage analysis, though this might be difficult to find out. |
Update: ignore this, I was mistaken about which signature is deprecated.Ow. Partial data from 2016-01-28 (partial because this is an exteremly simple search for anything that is named
Update: ignore this, I was mistaken about which signature is deprecated. |
@ChALkeR Most of them don't look like the one which we are trying to deprecate here :D As I said, it would be a little tricky to search. Probably something like |
@thefourtheye Ah, I misread the change, that was a mistake on my side. Sorry. I will update this soon. |
@thefourtheye Updated. Note that Once again: note that this is partial, because this is an exteremly simple search for anything that is named
|
@ChALkeR Now that you mentioned it, I am not sure if the change proposed in this PR would be fine. If someone is using |
@thefourtheye I think that we should add an explicit throw there, and leave the if-else branches as they are. |
Ya, that sounds better here. @mscdex also suggested the same. @trevnorris what do you think? |
@thefourtheye @ChALkeR @mscdex now throwing an exception -- looks better? |
LGTM, if CI and @trevnorris are okay with this. |
encoding = offset; | ||
offset = length >>> 0; | ||
length = swap; | ||
throw new Error('Buffer.write(string, encoding, offset[, length]) ' + |
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.
technically should be ``Buffer.write(string, encoding[, offset[, length]])'`, but I really don't care whether it's changed.
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.
string, encoding
fits into the current signature, so it's not deprecated. I made that mistake in the first grep :-).
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.
@ChALkeR Sorry, not following.
What's wrong with dropping the |
@trevnorris because then if someone is still using the obsolete form, say I can't see the CI because apparently it's on lockdown... LMK how it goes |
LGTM |
@dcposch I can buy that. Maybe just put a comment as to why we're throwing. Since in the normal case we'd just ignore any trailing arguments (only exception is the recent change to Buffer constructor, which now throws if encoding is passed with a number). |
@trevnorris done |
@dcposch Thanks for that. LGTM |
* Explit throw on deprecated Buffer.write(...) * Update tests, remove obsolete Buffer.write(...) * Add comment for obsolete Buffer.write(...) PR-URL: #5048 Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 2c55cc2 |
* Explit throw on deprecated Buffer.write(...) * Update tests, remove obsolete Buffer.write(...) * Add comment for obsolete Buffer.write(...) PR-URL: nodejs#5048 Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>
The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).
The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).
The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).
The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).
The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).
It said:
// XXX legacy write(string, encoding, offset, length) - remove in v0.13
And we're just a few versions past that :)
@mikeal