make JSON.parse return error positions #4373

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
Contributor

rlidwka commented Dec 24, 2013

See issue #3869. Since it is marked patch-welcome, here is a patch. :)

Setup:

$ echo '{ "name": "test", some garbage' > package.json

Was:

$ npm install
npm ERR! install Couldn't read dependencies
npm ERR! Failed to parse json
npm ERR! Unexpected token s
npm ERR! File: /tmp/package.json
npm ERR! Failed to parse package.json data.
npm ERR! package.json must be actual JSON, not just JavaScript.

Will be:

$ ~/npm/cli.js install
npm ERR! install Couldn't read dependencies
npm ERR! Failed to parse json
npm ERR! Unexpected token 's' at 1:19
npm ERR! { "name": "test", some garbage
npm ERR!                   ^
npm ERR! File: /tmp/package.json
npm ERR! Failed to parse package.json data.
npm ERR! package.json must be actual JSON, not just JavaScript.

Implementation:

It was suggested earlier to simply wrap JSON.parse in try..catch whereever it's used and use another library to figure out an exact error.

Sadly, it needs to be done for every JSON.parse call there is. Including those parsing npm-shrinkwrap.json and God knows what else. Ok, we can wrap it into it's own function.

Sadly, npm is scattered across multiple packages, so for example read-package-json will need to be updated as well. When I got to this point, I said "screw it, making minor pull requests to multiple packages is just stupid" and wrote a nice patch.

Monkeypatch warning:

Yes, I perfectly understand the implications of it. Every single library including core will have this fallback. But since it's only a fallback, what could possibly go wrong?

Disadvantages:

  1. If the error is deep inside json, parser will bloat up your stack trace with internal calls. Probably fixable with another try..catch.
  2. If somebody uses reviver (?) with side effects (??), it may be called x2 times. Maybe check for error type or something.

Tests:

None at the moment, but I can add some upon request.

Member

domenic commented Dec 29, 2013

Soooo I'm not terribly comfortable with this, although I admit it might be the better solution. But I think there aren't too many places JSON is parsed in npm... Well, OK, a find-in-files reveals a lot, but I am not sure how many of those matter. I would think the most important place would be in normalize-package-json, but also in shrinkwrap, as you point out... hmm.

We'll see what other maintainers think...

Contributor

alexgorbatchev commented Feb 12, 2014

👍

mgol commented Jan 3, 2015

I don't like the fact that it changes a global. This always leads to unpleasant surprises.

If JSON.parse is too little informative, it'd be better to switch to a library for parsing all over the code base. I know it's tedious but at least safe.

Another option would be to submit a PR to v8 to provide a more informative message. Firefox, for example prints:

SyntaxError: JSON.parse: expected double-quoted property name at line 1 column 19 of the JSON data

Still worse than what jju provides but who said that couldn't be tweaked further as well? And that's way better than what's currently in v8 anyway.

Contributor

smikes commented Jan 4, 2015

Since this has languished for over a year, I'm guessing it's a "no".

I have written a module that provides a drop-in replacement for JSON.parse() that calls jju.parse() as a fallback, and does no monkeypatching. PR for read-package-json is here: npm/read-package-json#42

Member

edef1c commented Jan 6, 2015

Maybe just export stringify too and make it a drop-in for the JSON object? It means we can add it all over the place easily with just var JSON = require(…)

Contributor

othiym23 commented Feb 27, 2015

I'm not comfortable with this kind of global monkeypatch either. This means that the npm tests have to try everything, because the many subsidiary modules don't have their tests written with this change in mind. npm's tests don't test everything (far from it), and as a result we're going to need a more targeted solution. Thanks for putting this together and sparking the discussion, though.

@othiym23 othiym23 closed this Feb 27, 2015

@smikes smikes referenced this pull request in npm/read-package-json Feb 27, 2015

Closed

use json-parse-helpfulerror #42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment