Deprecate legacy fs.read and fs.readSync arguments

fs.read and fs.readSync support a little-known alternative usage that lets you get back a String instead of Buffer.

// `fd` is a descriptor to a file that contains 'xyz'
fs.read(fd, 3, 0, 'utf-8',function(err, str, bytesRead) {
  // str is 'xyz'
});

var r = fs.readSync(fd, 3, 0, 'utf-8'); // returns ['xyz', 3] wut?

This doesn't match what node does elsewhere. While preparing this PR, I experienced the very real cost that supporting this old usage causes. It increases the surface area for bugs in node core, as well as user code.

Can we get stats on how much this is used?

Can we, at least as a start, start printing a deprecation warning for this usage?

Additionally, decoding UTF-8 for anything but an entire file makes little sense, unless you're aware of code point boundaries. Total footgun.

What about the "utf-8" string-type option that you can pass to createReadStream(..) (and createWriteStream(..) for that matter)? Is that similarly "legacy" to consider deprecating?

FWIW, there was a tutorial video released just today which uses the bare "utf8" (not "utf-8") string value with createReadStream(..).

A question of clarification: is the option really utf-8 instead of utf8? I've never seen it written as utf-8, and indeed the nodejs docs list it as utf8 (e.g. here).

I copied the 'utf-8' usage from the node.js tests, and that's indeed supported. I agree it's not ideal, but it's nowhere near as bad an undocumented/deprecated-but-not-really polymorphic fs.read function. Unless the node.js core folks disagree, I think removing 'utf-8' argument support is a separate discussion, for another issue.

@feross

What exactly are you suggesting deprecating @feross? The additional argument entirely? Just the utf8 option?

Usually the best path on these kinds of questions is to force the issue and submit a PR and you'll soon find out if there's any concerns, kind of like what's happening over in #4217.

On utf8 specifically I agree with @nathan7's point on boundaries but that doesn't apply to all possible encodings and perhaps there are good use-cases out there for this.

@chrisdickinson or @ChALkeR are either of you able to do a search on this to give us something to work with?

@rvagg

@rvagg This PR is exactly what I was suggesting: #4525

A quick and dirty search for cases where fs.read or fs.readSync is called with a second argument starting with numeric symbols or being a varible named length, count, num or size:

fs\.readSync\([^,\)]+,\s+([0-9]+|(length|count|num|size)[^a-zA-Z]):

brushtail-0.0.1.tgz/main.js:18:    content = fs.readSync(process.stdin.fd, 102400, 'utf8')[0];
buildjs-1.0.3.tgz/lib/build.js:52:                var sig = fs.readSync(fd, 25, 0, "ascii");
fastareader-0.2.0.tgz/test/test.js:30:  var read = fs.readSync(fd, 1 + id.length, result[id].start);
fastareader-0.2.0.tgz/test/test.js:42:  var read = fs.readSync(fd, 1 + result[id].linelen, result[id].start + result[id].idlen());
graphite-cli-0.1.10.tgz/lib/index.js:91:  argv.target || (size = fs.fstatSync(process.stdin.fd).size, trim(head(fs.readSync(process.stdin.fd, size)))));
json-optimize-0.1.0.tgz/tests/test-size.js:48:  //var response = fs.readSync(process.stdin.fd, 100, 0, "utf8");
showdown-1.2.3.tgz/src/cli/makehtml.cmd.js:70:      input = size > 0 ? fs.readSync(process.stdin.fd, size)[0] : '';
todo.txt-node-0.1.0.tgz/lib/wrapper.js:48:      response = fs.readSync(process.stdin.fd, 1024, 0, 'utf8');
verbotenjs-1.2.10.tgz/index.js:325:                var response = fs.readSync(process.stdin.fd, 500, 0, "utf8");
verbotenjs-1.2.10.tgz/index.js:429:                var response = fs.readSync(process.stdin.fd, 500, 0, "utf8");
verbotenjs-1.2.10.tgz/web/verboten.js:6975:                var response = fs.readSync(process.stdin.fd, 500, 0, "utf8");
verbotenjs-1.2.10.tgz/web/verboten.js:7079:                var response = fs.readSync(process.stdin.fd, 500, 0, "utf8");

fs\.read\([^,\)]+,\s+([0-9]+|(length|count|num|size)[^a-zA-Z]):

biojs-vis-blast-0.1.5.tgz/node/test/disabled/test-eio-race.js:65:    fs.read(fd, 1024, 0, 'binary', function(err, chunk, bytesRead) {
codius-engine-1.2.1.tgz/apis/fs/index.js:191:  fs.read(self._openedFds[fd], size, position, encoding, callback);
edp-zookeeper-3.4.5.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
flush-all-0.1.1.tgz/node-v0.13/test/disabled/test-eio-race.js:65:    fs.read(fd, 1024, 0, 'binary', function(err, chunk, bytesRead) {
geddy-13.0.8.tgz/lib/response/index.js:139:            fs.read(fd, 16 * 1024, pos, encoding,
izookeeper-0.1.0.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
jammit-express-0.0.1.tgz/init.js:46:    fs.read(fd, 100000, 0, "utf8", function(err, str, bytesRead) {
kuebk-zookeeper-3.4.15.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
libuv-fs-0.0.1.tgz/test/fs.js:52:                fs.read(fd, 33, 0, 1, -1);
light-node-zookeeper-0.1.0.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
meili-cli-watch-0.0.14.tgz/libs/restler/lib/multipartform.js:122:             fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
my-zookeeper-3.4.1-2.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
node-hdfs-0.0.1.tgz/demo/demo.js:43:  hdfs.read(hdfs_file_path, 1024*1024, function(reader) {
openapi-node-3.0.3.tgz/pakmanaged.js:58137:                   fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
pezhu-0.0.0.tgz/Downloads/node-v0.9.11/test/disabled/test-eio-race.js:65:    fs.read(fd, 1024, 0, 'binary', function(err, chunk, bytesRead) {
rest-in-node-0.1.0.tgz/lib/multipartform.js:115:              fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
restler-3.3.0.tgz/lib/multipartform.js:118:           fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
restler-aaronblohowiak-0.0.2.tgz/lib/multipartform.js:111:            fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
restler-b4030c6b67-2.0.0.tgz/lib/multipartform.js:115:                fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
restler-client-3.2.6.tgz/lib/multipartform.js:118:            fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
restless-0.0.11.tgz/lib/multipartform.js:117:         fs.read(fd, 1024 * 4, position, "binary", function (er, chunk) {
zookeeper-3.4.6-7.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
zookeeper-robskillington-3.4.3-3.4.3-1.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
zookeeper-rp-3.4.5-2.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);
zookeeper-uber-3.4.6-3-uber-bundled-iostop-yosemite.tgz/lib/test-promise.js:6:  return fs.read(fd, 4096);

As usual, has false negatives and checks only the latest versions of packages to the date when the dataset was build (that was on 2015-09-21).

@nathan7 i'd argue that it's safer to decode utf8 via the option, since internally it uses string_decoder, which you need to know about otherwise.

Closing since #4525 landed.

Navigation Menu

Repository navigation

Deprecate legacy `fs.read` and `fs.readSync` arguments #4530

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Deprecate legacy fs.read and fs.readSync arguments #4530

Description

Activity

edef1c commented on Jan 4, 2016

getify commented on Jan 4, 2016

saibotsivad commented on Jan 4, 2016

feross commented on Jan 5, 2016

rvagg commented on Jan 5, 2016

feross commented on Jan 6, 2016

ChALkeR commented on Jan 13, 2016

dominictarr commented on Feb 10, 2016

jasnell commented on Apr 3, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Deprecate legacy `fs.read` and `fs.readSync` arguments #4530