Repository navigation
Deprecate legacy fs.read and fs.readSync arguments #4530
Description
Activity
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-typeoptionthat you can pass tocreateReadStream(..)(andcreateWriteStream(..)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 withcreateReadStream(..).A question of clarification: is the option really
utf-8instead ofutf8? I've never seen it written asutf-8, and indeed the nodejs docs list it asutf8(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 polymorphicfs.readfunction. Unless the node.js core folks disagree, I think removing'utf-8'argument support is a separate discussion, for another issue.What exactly are you suggesting deprecating @feross? The additional argument entirely? Just the
utf8option?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?
A quick and dirty search for cases where
fs.readorfs.readSyncis called with a second argument starting with numeric symbols or being a varible namedlength,count,numorsize: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
utf8via the option, since internally it uses string_decoder, which you need to know about otherwise.
fs.readandfs.readSyncsupport a little-known alternative usage that lets you get back a String instead of Buffer.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?