fs: remove fs.read's string interface #9683

seishun · 2016-11-18T20:46:06Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

This interface has been deprecated (removed from the docs) for over 6 years (c93e0aa) and deprecated (runtime warning) for 2 major releases (1124de2).
Unlike things like require('sys') that have effectively zero cost, this interface creates significant maintenance burden (see fs: don't throw in read if buffer too big #3503, Investigate flaky test-fs-read-buffer-tostring-fail on Raspberry Pi #7042, test: fix flaky test/fs-read-buffer-tostring-fail #7575) and makes the code harder to follow, which raises the barrier for contributions (brought up by @feross in Deprecate legacy fs.read and fs.readSync arguments #4530; also affected me personally while investigating readFile(fd) does not read the whole file (at least on Windows, possibly other platforms) #9671).

Note that test-fs-read-zero-length.js and test-fs-read.js aren't modified - I deleted them and renamed test-fs-read-buffer-zero-length.js and test-fs-buffer-read.js, since the "buffer" part doesn't carry any meaning anymore.

ronkorving · 2016-11-25T05:27:30Z

Oh god, yes please 👍

claudiorodriguez · 2016-11-25T16:27:11Z

Should we not add a test that passes in a string instead of a buffer, and expects an error to be thrown (and throw that error in fs.read if the binding doesn't)?

seishun · 2016-11-25T18:18:31Z

@claudiorodriguez could you link some test that tests throwing on invalid arguments?

jasnell

LGTM with green CI

claudiorodriguez · 2016-12-06T09:22:39Z

@seishun off the top of my head, the one I wrote for dgram, https://github.com/nodejs/node/blob/master/test/parallel/test-dgram-createSocket-type.js

seishun · 2016-12-06T11:55:33Z

Added a test for throwing.

Fishrock123

This interface has been deprecated (removed from the docs) for over 6 years (c93e0aa) and deprecated (runtime warning) for 2 major releases (1124de2).

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

seishun · 2016-12-17T15:13:21Z

@nodejs/collaborators I'm going to land this in a week from now if there are no objections (or suggestions on how to get git blame to show the tests as renamed, rather than modified).

indutny

LGTM

thefourtheye · 2016-12-20T12:55:29Z

test/parallel/test-fs-read-type.js

+const fs = require('fs');
+const filepath = path.join(common.fixturesDir, 'x.txt');
+const fd = fs.openSync(filepath, 'r');
+const expected = 'xyz\n';


Strictly speaking, this is not the actual "expected", right?

What's your suggestion?

thefourtheye · 2016-12-20T13:01:06Z

test/parallel/test-fs-read-zero-length.js

-assert.equal(r[1], 0);
+const r = fs.readSync(fd, bufferSync, 0, 0, 0);
+assert.deepStrictEqual(bufferSync, Buffer.alloc(0));
+assert.equal(r, 0);


Please use strictEqual.

I'd prefer to keep test files unrelated to this PR unmodified, since I'm not familiar with their finer details.

thefourtheye · 2016-12-20T13:06:02Z

test/parallel/test-fs-read.js

-          assert.ifError(err);
-          assert.strictEqual(str, expected);
-          assert.strictEqual(bytesRead, expected.length);
+        common.mustCall(function(err, bytesRead) {


We will be getting the buffer object as the third argument. You might want to check if it is the original bufferAsync object itself.

thefourtheye · 2016-12-20T13:07:54Z

Despite three CTC LGTMs, tagging @nodejs/ctc as it is a major change.

It is a maintenance burden that was removed from the docs in 2010 (c93e0aa) and runtime-deprecated in v6.0 (1124de2).

seishun · 2017-01-08T17:13:44Z

Since there were no comments from CTC in 19 days, I assume everyone is okay with this change. Merging in 24 hours.

jasnell · 2017-01-09T04:59:25Z

with 3 lgtm's from ctc members, you should be good to go on landing

It is a maintenance burden that was removed from the docs in 2010 (c93e0aa) and runtime-deprecated in v6.0 (1124de2). PR-URL: nodejs#9683 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 18, 2016

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 18, 2016

jasnell approved these changes Dec 5, 2016

View reviewed changes

Fishrock123 approved these changes Dec 16, 2016

View reviewed changes

indutny approved these changes Dec 17, 2016

View reviewed changes

benjamingr approved these changes Dec 17, 2016

View reviewed changes

thefourtheye reviewed Dec 20, 2016

View reviewed changes

seishun added 3 commits January 8, 2017 18:56

fs: remove fs.read's string interface

10f4734

It is a maintenance burden that was removed from the docs in 2010 (c93e0aa) and runtime-deprecated in v6.0 (1124de2).

doc: remove redundant "buffer" from test names

61fcb21

add test

1f4bd52

nodejs-github-bot added the dont-land-on-v7.x label Jan 8, 2017

seishun merged commit 3c2a936 into nodejs:master Jan 10, 2017

joyeecheung mentioned this pull request Jan 21, 2017

assert: enforce type check in deepStrictEqual #10282

Closed

3 tasks

targos removed the dont-land-on-v7.x label Jan 28, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

seishun deleted the remove-legacy-read branch November 12, 2017 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: remove fs.read's string interface #9683

fs: remove fs.read's string interface #9683

seishun commented Nov 18, 2016 •

edited

Loading

ronkorving commented Nov 25, 2016

claudiorodriguez commented Nov 25, 2016

seishun commented Nov 25, 2016

jasnell left a comment

claudiorodriguez commented Dec 6, 2016

seishun commented Dec 6, 2016

Fishrock123 left a comment

seishun commented Dec 17, 2016

indutny left a comment

thefourtheye Dec 20, 2016

seishun Jan 8, 2017

thefourtheye Dec 20, 2016

seishun Jan 8, 2017

thefourtheye Dec 20, 2016

thefourtheye commented Dec 20, 2016

seishun commented Jan 8, 2017

jasnell commented Jan 9, 2017

fs: remove fs.read's string interface #9683

fs: remove fs.read's string interface #9683

Conversation

seishun commented Nov 18, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

ronkorving commented Nov 25, 2016

claudiorodriguez commented Nov 25, 2016

seishun commented Nov 25, 2016

jasnell left a comment

Choose a reason for hiding this comment

claudiorodriguez commented Dec 6, 2016

seishun commented Dec 6, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

seishun commented Dec 17, 2016

indutny left a comment

Choose a reason for hiding this comment

thefourtheye Dec 20, 2016

Choose a reason for hiding this comment

seishun Jan 8, 2017

Choose a reason for hiding this comment

thefourtheye Dec 20, 2016

Choose a reason for hiding this comment

seishun Jan 8, 2017

Choose a reason for hiding this comment

thefourtheye Dec 20, 2016

Choose a reason for hiding this comment

thefourtheye commented Dec 20, 2016

seishun commented Jan 8, 2017

jasnell commented Jan 9, 2017

seishun commented Nov 18, 2016 •

edited

Loading