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

Better emulate jQuery by resolving successful request on JSON parse error #46

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

JoshuaCWebDeveloper
Copy link

In jQuery: when a successful ajax requests with datatype json has a body that can't be parsed, jQuery will replace the data with a custom error object and treat it as a successful request (resolve promise).

This has the effect of ignoring JSON parse errors for unsuccessful quests (e.g. a 401 where the response data is never passed to a handler).

Currently, najax will instead reject the promise, treating it as an unsuccessful request.

This has the effect of throwing a parse error when an unsuccessful request fails to return JSON, meaning the actual request error is covered up.

@JoshuaCWebDeveloper
Copy link
Author

Removing the extra call to najax in my last commit seems to eliminate the StandardJS error, but I don't know why it would be mistaking the url on line 141 for a comment in the first place...

@alanclarke
Copy link
Contributor

heya thanks for this, will take a look

// https://github.com/najaxjs/najax/commit/54cfac6aa0b7cf8a89efae0f39d1f054c8859de0
// commenting out the 'default to "GET"' test also fixes this)
// make an extra call to najax in order to clear it
najax({url: 'http://www.example.com'})
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry about this, we should probably clear all the nocks after each test?

e.g.

afterEach(function () {
   nock.cleanAll()
})

afterEach(function () {
nock.cleanAll()
})

Choose a reason for hiding this comment

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

I elected to do this globally since we were dealing with contamination between describe blocks.

@demersus
Copy link
Contributor

From my local testing with jQuery, I am getting the fail/error callback when it can't parse json. I have addressed some issues in my PR here: #56 for responding similarly to jQuery.

@alanclarke
Copy link
Contributor

@demersus @JoshuaCWebDeveloper is this superseded by #56?

@JoshuaCWebDeveloper
Copy link
Author

I have confirmed that jQuery will reject the promise if a successful request comes back with invalid JSON (@demersus was correct here). However, the original problem was that an error from the request is superseded by the JSON parse error. It appears that #56 does not fix that issue.

You can see in this test: https://jsfiddle.net/gvyfsy6h/14/

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.

4 participants