Ambiguous error when parsing some of falsy values

[Hoishin]

When I parsed undefined, the error was

JSONError: Cannot read property 'length' of undefined
    at module.exports ((...)node_modules/parse-json/index.js:26:19)

For NaN

JSONError: txt.slice is not a function

It would be more friendly error if it just says something like don't give me undefined/NaN! (in more formal way, of course)

I don't know if this has mentioned before, but at least I could not find related issues.

[Hoishin]

I opened PR on json-parse-better-errors, but I haven't got response for 5 days even though the owner seems to be active.
How long should I wait before opening PR in this repo?

(Sorry if this is a weird question, by the way. I am new to open source contribution.)

[sindresorhus]

I opened PR on json-parse-better-errors, but I haven't got response for 5 days even though the owner seems to be active.

Maintainers are often busy with lots of repos and can't always prioritize reviewing PRs. Give it some time. To maximize your chances of getting it merged, try to make the minimal amount of changes to fix the problem, and don't forget to add tests ;)

I would for example, throw early, so you don't have to wrap the whole existing code in an else statement:

if (typeof txt !== 'string') {
  const type = typeof txt === 'undefined' ? ' ' : ` the ${typeof txt} `
  e.message = `Cannot parse${type}${String(txt)}`
  throw e;
}

I'm also not sure why you're special-casing undefined.

[Hoishin]

Thanks for the advice
I think I will add some tests meanwhile. And yes throwing early surely is better, I don't know why I didn't do that xD

As for special-casing undefined, I thought it would be informative to throw with messages like Cannot parse the number Infinity because it is an error due to an invalid type. But it doesn't look good to say Cannot parse the undefined undefined, so I just made it say Cannot parse undefined.

[sindresorhus]

Cannot parse the number Infinity

I don't see the point in mentioning that Infinity is a number. I would go with:

Cannot parse Infinity

[sindresorhus]

Actually, why even do the check after parsing. We know up front those types are not valid, so you can just throw a TypeError on line 5.

[Hoishin]

I agree that Infinity is clearly number and doesn't need its type mentioned.
After a while of exploration in JavaScript world, only convincing reason to mention the type I've found is an empty array [], which would be an error with the message Cannot parse . Probably not worth mentioning types then.

The reason I put the check after parsing is to preserve performance for valid strings. Would it be still better to always check the type before parsing?

[sindresorhus]

only convincing reason to mention the type I've found is an empty array [], which would be an error with the message Cannot parse. Probably not worth mentioning types then.

I would rather just special-case this one, with:

Cannot stringify empty array

---

The reason I put the check after parsing is to preserve performance for valid strings. Would it be still better to always check the type before parsing?

That's a good point. Yeah, makes more sense to optimize for the common-case.

[Hoishin]

Yeah the special-case message looks good. Thank you so much for the help.

[Hoishin]

@sindresorhus The fix is now merged to json-parse-better-errors and released as v1.0.2. Since it's patch version it should automatically be fixed for this package.

[sindresorhus]

Great! Thanks for working on it :)