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

Stop jhr XHR function from swallowing JSON.parse error #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

silasdavis
Copy link

@silasdavis silasdavis commented May 30, 2019

There are a number of circumstances where the server may return a non-empty non-JSON response:

  • Connection refused
  • Bad origin
  • Failure to reverse proxy
  • Other middleware

In these cases jhr dies silently and so the front end cannot easily display an error message. This change wraps the JSON parsing in a function with a try catch and returns an appropriate error.

I have an issue with the tests - the pass successfully in the browser (by loading runner.html) but they fail with yarn test which complains with Failed assertion: expected: [object Object], but was: [object Object]. This is different to failure in the browser UI that gives you a nice string diff between the objects. it seems like gulp-qunit is ending up with a different broken implementation of deepEqual. I tried upgrading gulp-qunit and qunit but it didn't help. Perhaps you have an idea how to fix, otherwise we can work around it some other way. It's odd because I don't see the difference between the other usages that work fine.

@cainlevy
Copy link
Member

cainlevy commented Jun 1, 2019

I think it's a legacy issue in PhantomJS. The console logger isn't as capable.

Speaking of PhantomJS issues -- the expected error is different, too:

HTTP response 'Origin is not a trusted host.' is not JSON as expected: SyntaxError: JSON Parse error: Unexpected identifier "Origin"

@@ -1,3 +1,4 @@
node_modules
lib/*
dist
.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider handling this locally via a global gitignore: https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/

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

Successfully merging this pull request may close these issues.

3 participants