fs: introduce readJSON functions

[targos]

This adds fs.readJSON, fs.readJSONSync, fsPromises.readJSON, and
fileHandle.readJSON.
All of them take the usual parameters of their respective readFile
equivalents, with the addition of JSON.parse's reviver option.
The file will be read as a string, with the encoding defaulting to
'utf-8', and if the read succeeds, JSON.parse will be called on the
result. Parsing errors are propagated the same way as reading errors.

[targos]

/cc @nodejs/fs

[addaleax]

I'm kind of +/-0 on this, but would we maybe consider only providing this for the fsPromises variants to encourage using those?

[targos]

would we maybe consider only providing this for the fsPromises variants to encourage using those?

I initially considered this but implemented all of them because I was afraid of people asking for consistency. Happy to change that if we all agree.

[mcollina]

Good work, I think we should add these as they are convenient.

[jasnell]

I'd be fine with dropping the legacy fs variants and keeping only the promise version, and even then I'd be happy keeping only the 'filehandle.readJSON()` variant.

[targos]

I'd be fine with dropping the legacy fs variants and keeping only the promise version, and even then I'd be happy keeping only the 'filehandle.readJSON()` variant.

I think fsPromises.readJSON() is still useful as a simple way to read and parse a JSON file in one line.

[cjihrig]

would we maybe consider only providing this for the fsPromises variants to encourage using those?

Since the code is already written, it would be nice to keep them - at least the synchronous version.

[targos]

Updated, PTAL.

[nodejs-github-bot]

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

[devsnek]

what's the motivation for this exactly?

[targos]

It is common to read and parse JSON in Node.js applications. However, instead of using the fs module, people tend to instead require() the files because it's simpler.
The populare fs-extra module provides a very similar functionality.

[devsnek]

i think people use require because of the relative path resolution. either way though i'm not blocking this, just seems a bit derived.

[tniessen]

There's certainly a discussion to be had about how many pure convenience APIs our core should provide.

On the other hand, if we implemented this using streaming JSON parsing, this would actually have benefits over typical implementations, especially memory-wise, but also when it comes to safety. For example, the current implementation would hang and eventually crash the process for readJSON('/dev/zero'), whereas a streaming implementation would result in virtually no resource usage and an immediate parsing error.

[jasnell]

For the most part, coming back to this, I'm generally -0 on it. I wouldn't block it but I don't think it adds much value overall. It would be a different matter if it were part of a web platform compatibility story but as it is, I think it's better left for userland.

[targos]

Closing as there seems to be no support from collaborators.

[tniessen]

I think I'd still support something like this if it was significantly better than readFile + JSON.parse, e.g., by failing early if there is a syntax error (see #37944 (comment)).

[targos]

I don't have the time to work on something like that.