let's add input type checks for "data" argument

[revelt]

Thank you for creating and maintaining this library. I want to report a following issue.

Currently, "data" argument is checked only for existence:
https://github.com/sindresorhus/write-json-file/blob/master/index.js#L15

If it exists, it is assumed that is it a plain object. The problem is, accidentally the data can come as string too and this library will silently write an empty file! For example, format-package returns a string and that's why write-json-file in one of my libraries was silently writing empty files.

Thinking about this, we could either:

1. always throw if data is not a plain object,

or, more cheeky option, do this:

2. if there are any processing options set, attempt to parse the string, try-catch and throw a meaningful error message if it's not a JSON string; ELSE (no processing), pipe all inputs straight into write-file-atomic, skipping JSON.stringify.

Arguments for the second option are the following.

• Maybe somebody already has stringified JSON and just wants to write it do disk? Installing write-file-atomic as a new dependency is tedious, plus, it's a possible liability if directly tapped version of write-file-atomic mismatches the one that write-json-file uses. Promisifying write-file-atomic is another liability — what if user does not read the source and tap another promisifying library, other than pify?

• In format-package case where data is string, it would be terribly inefficient to JSON.parse the data only to be right away stringified by write-json-file.

• Second option is also more user-friendly: we could attempt to parse data (if processing is requested) and that would not require any extra dependencies.. And if there is no processing requested, we could cut corner and skip JSON.stringify.

• Also, the first option would require plain object checks which is a new dependency like lodash.isplainobject. Going 2nd option route we avoid this, we just attempt to parse if string is given, else throw. That's no extra dependencies.

Either way, even if we do nothing about this, at least let's write a unit test and cover the case, what happens when data is: 1) empty string, 2) non-empty string but not JSON, 3) stringified valid JSON.

[sindresorhus]

Thanks for reporting!

The main problem is that a top-level string is actually a valid JSON file. "foo" is valid JSON. You could also have a top-level array. So the best solution would be to skip the sorting when not an object.

Handling already stringified JSON is outside the scope of this module.

[revelt]

@sindresorhus speaking about solution, would you be happy if we added lodash.isplainobject check on the input arguments and threw if not?

Row 15 onwards would become:

if (data === undefined) {
  throw new TypeError('Expected data to stringify');
} else if (!isObj(data)) {
  throw new TypeError('Expected a plain object');
}

?

[sindresorhus]

I think you misunderstood my previous comment. We can't throw if it's not an object as it's still valid JSON. What we can do is to guard the sorting logic behind an if-statement that checks whether it's an object, but not array (we don't check for plain object, as normal objects should be allowed too).

[revelt]

it's embarrassing but I just learned JSON content can be of array-type...