Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid JSON format on toepoke.co.uk manifest.json #1702

Closed
toepoke opened this issue Feb 12, 2017 · 11 comments
Closed

Invalid JSON format on toepoke.co.uk manifest.json #1702

toepoke opened this issue Feb 12, 2017 · 11 comments
Assignees

Comments

@toepoke
Copy link

toepoke commented Feb 12, 2017

Hi,

When running the report I get the following error:

"ERROR: file isn't valid JSON: SyntaxError: Unexpected token  in JSON at position 0"

It's been driving me mental for weeks! Turns out it's because the manifest.json was saved with a byte-order-mark (I'm on Windows). Saved without the BOM and all works well.

May I suggest adding a warning to the documentation so others don't have a similar battle?

Many thanks,
Franz.

@XhmikosR
Copy link
Contributor

As far as I can tell LH is using the standard JSON.parse method.

Might be worth looking into https://github.com/sindresorhus/strip-bom/blob/master/index.js

@brendankenny
Copy link
Member

brendankenny commented Feb 13, 2017

Not sure what the correct thing to do here is. A BOM is an invalid character in JSON, but in some cases clients will drop the BOM before attempting to parse.

At the very least we could warn that the first character is a BOM to give a better error message, though.

If I test (on a non-Windows machine) a manifest saved with a BOM, the application panel in devtools lists no information for the manifest. Since IIUC devtools is exposing what Chrome has actually parsed from the manifest, this implies Chrome doesn't understand the manifest either (devtools does not report a JSON SyntaxError, though, or any error at all).

If the browser doesn't understand it, we should definitely mark it as an error, just with a better error message.

OTOH, (one of?) the native Chrome JSON parser(s) explicitly skips BOMs when encountered. Not sure how that interacts with the manifest parser.

@toepoke
Copy link
Author

toepoke commented Feb 14, 2017

HI @brendankenny,

To confirm when I was saving with BOM I didn't see the manifest parameters in DevTools. They magically start appearing when saved without BOM - which is in line with what you're seeing.

If it's not feasible to raise an error, perhaps a warning that always appears in the report would be sufficient? I'm just conscious of others falling into the same issue.

If I can be of any assistance, please give me a shout.

@XhmikosR
Copy link
Contributor

IMO, we shouldn't do something special. It's an error in the parser so Chrome will not load it as I understand it.

@paulirish
Copy link
Member

paulirish commented Mar 3, 2017

@toepoke when you look at the manifest panel in devtools, do you see warnings or errors?
image

also same question for @zmandel, as it seems like a related issue.

@zmandel
Copy link

zmandel commented Mar 3, 2017 via email

@toepoke
Copy link
Author

toepoke commented Mar 5, 2017

HI @paulirish ,

Nothing appears in the console. The manifest is devtools is just blank when saved with BOM:

Hope this helps.

Error state:
failure

Working state:
working

@wardpeet
Copy link
Collaborator

wardpeet commented Mar 5, 2017

perhaps it's more like a chrome issue where it should tell that it's invalid.

@HolgerJeromin
Copy link

There is an open issue for the dev tools where i added the comment that there should be an info text (would have saved us a few debug hours :-)

@wardpeet
Copy link
Collaborator

wardpeet commented Apr 21, 2017

@HolgerJeromin thank you for reporting to devtools.

I'll update lighthouse to check <0xEF 0xBB 0xBF> as first bytes to show a better error message

@wardpeet wardpeet self-assigned this Apr 23, 2017
wardpeet added a commit that referenced this issue May 7, 2017
wardpeet added a commit to wardpeet/lighthouse that referenced this issue May 7, 2017
wardpeet added a commit to wardpeet/lighthouse that referenced this issue May 7, 2017
wardpeet added a commit to wardpeet/lighthouse that referenced this issue May 12, 2017
wardpeet added a commit to wardpeet/lighthouse that referenced this issue May 29, 2017
@wardpeet
Copy link
Collaborator

Fixed on master

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

No branches or pull requests

7 participants