Add throws option for readFile (async)

[maxkorp]

This maintains the behavior of readFileSync.
Just like an error actually reading the file would still throw, in this case it will still return the error to the callback.
If an error happens parsing the contents, the callback will no longer recieve an error, but null will be passed back to the callback for the object, just as the sync version returns null if it fails to parse. If the option is omitted (or still true) existing behavior will be maintained.

[jprichardson]

Thanks for your interest.

1. This won't get accepted without a test. This statement doesn't imply that it will get accepted if you add a test case.
2. What's the use case? Why not just ignore the error if it's a parse error?

[maxkorp]

1. I can happily add a test case, although we should discuss the second point first to see if you think it's worth it.
2. There are two points to this that I can think of. The first smaller one is simply consistency. If i can bypass error handling in the synchronous function, why not in the asynchronous case. Hardly a complete justification, but there's definitely something to be said for maintaining consistency. I actually only realized it wasn't there already because I suggested using it (via fs-extra) to somebody whos code I was reviewing to tighten up a bit, assuming that readJsonSync worked identically to readJson the same way the rest of the sync methods do to their respective async methods. When that didn't work, I took a look and realized it only worked for the sync code and not the async code. The consistency definitely makes it easier to translate code from synchronous land to async land and vice versa.

The bigger thing (and this is still not a huge thing, but it's definitely a nicety) is related to use with promises, especially when used with compiled es6 code using default parameters (we use a lot of babel).

in a promise chain, assuming this has been promisified, we get (in its most condensed format)

.then(() => {
  return jsonFile.read('someFilePath')
    .catch(() => { return {}; });
})
.then((jsonOutput) => {
  //do some stuff
})

There are a few ways to write that clearly, but that's the most condensed way. With the additon of the option for throws: false, we can condense to

.then(() => {
  return jsonFile.read('someFilePath', {throws: false})
})
.then((jsonOutput = {}) => {
  //do some stuff
})

which while not light years apart, avoids the weird, potentially unclear-in-intention catch statement.

I realize fully that it's not a huge loss having the weird catch, but calling JSON.parse on the output of an fs.readFile isn't really either. More of just a convenience added, which is really the whole point of the module, no?

[jprichardson]

Ok, I think you make a good case on both points. I definitely start concepts with sync and then move to async.

I'm starting to think that maybe throws is a horrible name. By its name, it could lead a person to think that readFileSync will never throw an error, even if there's a file system error. The name should be something clearer. errorOnParse? What are your thoughts? If it was errorOnParse, that might not signal to the coder that it will not throw in the sync case if there's a JSON parse error. I don't know, but I definitely agree with the consistency aspect and I'd like the intent of the programmer to be crystal clear.

[maxkorp]

Sorry I'm just coming back to this.
Yeah I agree re:naming, although I'd say leave throws in for a while just to avoid breaking the api too badly. That said, adding a note to the docs about it and specifying it will still pass along the error if its not a parser error would be good.

I'll have a little time later to finally add a test as well 👍

[maxkorp]

By later I totally meant like half an hour later, apparently.

[jprichardson]

Excellent, thank you!

Why the name errorOnFailedParse as opposed to errorOnParse?

[maxkorp]

I can fix that, I just didn't remember what the term was off the top of my head. Can fix 👍

[maxkorp]

Although on second hand, I don't know that 'errorOnParse' is exactly clear, since it doesn't error when you parse, it passes along the error when the parse fails. Hrm. I mean, I'll yield to your judgement on that, but errorOnParse doesn't seem super clear (although errorOnFailedParse seems way too verbose).

[jprichardson]

Haha, I feel the same way. I'm open to another name?

On Thursday, December 17, 2015, Max Korp [email protected] wrote:

Although on second hand, I don't know that 'errorOnParse' is exactly
clear, since it doesn't error when you parse, it passes along the error
when the parse fails. Hrm. I mean, I'll yield to your judgement on that,
but errorOnParse doesn't seem super clear (although errorOnFailedParse
seems way too verbose).

[image: naming things]
https://camo.githubusercontent.com/a27ad78cca9d81f2b3a78a1825d8fc3d515230fa/687474703a2f2f692e696d6775722e636f6d2f5670434e476d362e6a7067

—
Reply to this email directly or view it on GitHub
#39 (comment)
.

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow JP Richardson on Twitter: https://twitter.com/jprichardson

[maxkorp]

if it wasn't an inversion to the api i'd suggest something like safeParse or something, to indicate that it won't error out when parsing.

What about something like passParsingErrors or so?

[maxkorp]

Ran with that, as it was the best I could come up with.

[jprichardson]

Sorry for the incredibly long delay, thanks for your work on this.

[jprichardson]

Published. However, please don't use passParsingErrors since the name will change. I've purposely left it undocumented. Use throws instead. I'm not sure which name I'll settle on, but if you use passParsingErrors, your code will break in the future. Thanks for the help again!