Inconsistency between readJson and readJsonSync when file does not exist

[LeoFrachet]

The throws option has an inconsistent behavior between readJson and readJsonSync when the file do not exist:

• fs.readJsonSync('path', { throws: false}); do not throw if the file do not exist;
• fs.readJson('path', { throws: false}); do throw if the file do not exist.

I would expect a consistent behavior between the two.

Test code:

console.log('Trying to load a non existing file using readJson/readJsonSync:');

try {
  fs.readJsonSync('I do not exist.json');
  console.log('readJsonSync does NOT throw.');
} catch (e) {
  console.log('readJsonSync throws.');
}

try {
  fs.readJsonSync('I do not exist.json', { throws: false });
  console.log('readJsonSync (with throws = false) does NOT throw.');
} catch (e) {
  console.log('readJsonSync (with throws = false) throws.');
}

fs.readJson('I do not exist.json').then(() => {
  console.log('readJson does NOT throw.')
}).catch((e) => {
  console.log('readJson throws');
});

fs.readJson('I do not exist.json', { throws: false }).then(() => {
  console.log('readJson (with throws = false) does NOT throw.')
}).catch((e) => {
  console.log('readJson (with throws = false) throws');
});

Outputs:

Trying to load a non existing file using readJson/readJsonSync:
readJsonSync throws.
readJsonSync (with throws = false) does NOT throw.
readJson throws
readJson (with throws = false) throws

Versions:

• Operating System: macOS 10.13.2 (17C88)
• Node.js version: 8.0.0
• fs-extra version: 5.0.0

[RyanZim]

@jprichardson This inconsistency is actually documented: https://github.com/jprichardson/node-jsonfile#readfilefilename-options-callback Do you know why you built it this way?

[jprichardson]

@RyanZim I don't remember... I do remember the option was only in the sync version first. There's a lot of history though in the jsonFile repo on this IIRC - https://github.com/jprichardson/node-jsonfile/search?q=throws&type=Issues&utf8=%E2%9C%93

Generally, I think APIs should be consistent and only break this for good reason - at this moment, I'm not seeing a good reason for the inconsistency.

[RyanZim]

So it seems throws was built for sync, because it's a pain to stop sync errors. It was added to async because someone didn't want to have to check if it was a fs-level error in the async version. Honestly, a throws option that silences all errors doesn't make sense for an async function. We could rename the async option to something more descriptive, but that would make the sync and async functions have different options. Thoughts?

[aprilandjan]

@RyanZim I think it is pretty reasonable if we specify { throws: false } with the async calls, because it's user's purpose to silence the erros, the async just makes the code running unblockly. Please reconsider that to make the behavior more consistent 🙏