fs: deprecate fs.read's string interface by thefourtheye · Pull Request #4525 · nodejs/node

thefourtheye · 2016-01-04T06:41:19Z

fs.read supports a deprecated string interface version, which is
not documented. It was intended to be deprecated in this commit in 2010
c93e0aa
This patch issues a deprecation message saying the usage of this
interface is deprecated.

jasnell · 2016-01-04T17:16:59Z

LGTM

thefourtheye · 2016-01-04T17:32:58Z

@jasnell Can you please have a look at it again? I missed the readSync part. I force pushed that as well now.

jasnell · 2016-01-04T17:36:50Z

heh... I forgot about that one too.. still LGTM but I'd like to get other opinions before landing

thefourtheye · 2016-01-04T17:39:25Z

@jasnell Thanks :-)

davidvgalbraith · 2016-01-05T09:13:21Z

Can we add a suggested course of action to the deprecation message? Something like "Use the Buffer API instead" and maybe a link to the relevant docs.

feross · 2016-01-06T17:07:15Z

LGTM

thefourtheye · 2016-01-11T06:50:24Z

@davidvgalbraith I updated as per your suggestion.

cc @nodejs/ctc

davidvgalbraith · 2016-01-11T07:43:41Z

LGTM

trevnorris · 2016-01-11T19:37:01Z

LGTM

ChALkeR · 2016-01-13T01:07:40Z

See #4530 (comment).
LGTM.

jasnell · 2016-01-13T16:21:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1218/

`fs.read` supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 nodejs@c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated.

thefourtheye · 2016-02-01T12:54:13Z

@nodejs/collaborators I'll land this tomorrow if there are no objections.

bnoordhuis · 2016-02-01T13:00:51Z

No objections. LGTM.

`fs.read` supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated. PR-URL: #4525 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

thefourtheye · 2016-02-05T04:17:52Z

Thanks for the reviews. Landed in 1124de2

feross · 2016-02-05T04:23:29Z

Nice work!

MylesBorins · 2016-02-05T08:54:51Z

This commit has broken npm on master.

To reproduce simply try to run npm install

Thankfully npm substack, npm visnup, and npm xmas are unaffected

Npm easter eggs aside I think that this commit should be backed out of master until a solution is found.

trevnorris · 2016-02-05T10:09:52Z

Unfortunate that we're tied down to a function signature that isn't even documented anymore.

thefourtheye · 2016-02-05T10:12:42Z

We tried to figure out the actual problem and our findings are at #5097 (comment)

feross · 2016-02-05T10:24:53Z

So, this wasn't causing the problem with npm after all?
On Fri, Feb 5, 2016 at 02:13 thefourtheye notifications@github.com wrote:

We tried to figure out the actual problem and our findings are at #5097
(comment)
#5097 (comment)
—
Reply to this email directly or view it on GitHub
#4525 (comment).

rvagg · 2016-02-08T06:21:12Z

Short term proposed solution: back this out completely on v5.x and force-push. We need that branch to be stable and happy for our releases this week and I've got no qualms with force pushing to non-master branches, does anyone here object? We do it to v4.x-staging occasionally as we run into trouble there.

thefourtheye · 2016-02-08T06:32:46Z

@rvagg This commit hasn't landed in 5.x yet. So that should be fine. Should we tag this as do-not-land-5.x or something?

ChALkeR · 2016-02-08T06:33:15Z

@rvagg That got to 5.x? I was talking about master only. If that got into 5.x, I do not object force-pushing to revert it. But wasn't it semver-major?

MylesBorins · 2016-02-08T06:35:36Z

@thefourtheye is correct, this is not in v5.x

If the fix is still giving people goosebumps and it not going to quickly land on master I'd like to see this change backed out so that we can have a working copy of npm on master... not having it is making smoke testing a bit of a pain (and arguably less reliable as it is running off an fork)

rvagg · 2016-02-08T06:43:21Z

k, sorry, ignore my comment then!

This reverts commit 1124de2 which landed in nodejs#4525 This commit has broken both npm + node-gyp How did it break npm? Deprecating fs.read's string interface includes `internal/util` Currently npm's dep tree includes an old version of graceful-fs that is monkey patching fs. As such everything explodes when trying to require `internal/util` nodejs#5102 is waiting for review but has not landed. We should revert ASAP to fix master while we decide what to do.

This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module. To be reverted in Node.js 7.0. Fixesnodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525.

This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module. To be reverted in Node.js 7.0 Fixes: #5097, see also #1898, #2026, and #4525. PR-URL: #5102 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module. To be reverted in Node.js 7.0 Fixes: nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525. PR-URL: nodejs#5102 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`fs.read` supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 nodejs@c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated. PR-URL: nodejs#4525 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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

thefourtheye added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 4, 2016

thefourtheye force-pushed the fs-read-legacy-string-way-deprecation branch from f0f85f8 to fb3c18eCompare January 4, 2016 17:31

feross mentioned this pull request Jan 6, 2016
Deprecate legacy fs.read and fs.readSync arguments #4530
Closed

thefourtheye force-pushed the fs-read-legacy-string-way-deprecation branch from fb3c18e to 8d31271Compare January 11, 2016 06:49

fs: deprecate fs.read's string interface
deb15fc
`fs.read` supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 nodejs@c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated.

thefourtheye force-pushed the fs-read-legacy-string-way-deprecation branch from 8d31271 to deb15fcCompare February 1, 2016 12:42

thefourtheye closed this Feb 5, 2016

thefourtheye deleted the fs-read-legacy-string-way-deprecation branch February 5, 2016 04:17

MylesBorins mentioned this pull request Feb 5, 2016
upgrade npm to 3.7.1 #5097
Closed

MylesBorins mentioned this pull request Feb 9, 2016
Revert "fs: deprecate fs.read's string interface" #5163
Closed

MylesBorins mentioned this pull request Feb 9, 2016
test: add test-npm-install to parallel tests suite #5166
Closed

jasnell mentioned this pull request Mar 17, 2016
Planning for v6 #5766
Closed

jasnell mentioned this pull request Apr 19, 2016
What is new in v6? #6264
Closed

jasnell mentioned this pull request Apr 26, 2016
2016-04-26, Version 6.0.0 (Current) Release #6383
Closed

julienma mentioned this pull request Oct 5, 2016
Deprecation warning on newer versions of Grunt DiscoverGrunt/grunt-http-upload#12
Closed

Uh oh!

Uh oh!

Conversation

thefourtheye commented Jan 4, 2016

Uh oh!

jasnell commented Jan 4, 2016

Uh oh!

thefourtheye commented Jan 4, 2016

Uh oh!

jasnell commented Jan 4, 2016

Uh oh!

thefourtheye commented Jan 4, 2016

Uh oh!

davidvgalbraith commented Jan 5, 2016

Uh oh!

feross commented Jan 6, 2016

Uh oh!

thefourtheye commented Jan 11, 2016

Uh oh!

davidvgalbraith commented Jan 11, 2016

Uh oh!

trevnorris commented Jan 11, 2016

Uh oh!

ChALkeR commented Jan 13, 2016

Uh oh!

jasnell commented Jan 13, 2016

Uh oh!

thefourtheye commented Feb 1, 2016

Uh oh!

bnoordhuis commented Feb 1, 2016

Uh oh!

thefourtheye commented Feb 5, 2016

Uh oh!

feross commented Feb 5, 2016

Uh oh!

MylesBorins commented Feb 5, 2016

Uh oh!

trevnorris commented Feb 5, 2016

Uh oh!

thefourtheye commented Feb 5, 2016

Uh oh!

feross commented Feb 5, 2016

Uh oh!

rvagg commented Feb 8, 2016

Uh oh!

thefourtheye commented Feb 8, 2016

Uh oh!

ChALkeR commented Feb 8, 2016

Uh oh!

MylesBorins commented Feb 8, 2016

Uh oh!

rvagg commented Feb 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants