-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(AjaxObservable): support json responseType on IE #1589
Conversation
- IE does not support json responseType, internally parse it into JSON object closes ReactiveX#1381
I'm not clear at the moment if this approach is legit & acceptable, send out as PR for discussion to close out issue. If it's expected to not to support those and let it be handled externally, I'll simply close this PR as well as issue. /cc @trxcllnt also for visibility. |
switch (this.responseType) { | ||
case 'json': | ||
if ('response' in xhr) { | ||
this.response = xhr.response; | ||
//IE does not support json as responseType, parse it internally | ||
this.response = xhr.responseType ? xhr.response : JSON.parse(xhr.response || xhr.responseText || ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we run this across all browsers? It seems legit on the surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've ran over Chrome / IE, can check FF (but not able to verify on safari).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this logic itself works on FF as well. (noticed response is null actually, but this seems somewhat different issue - api itself, or somewhere else that need to triage separately)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fallback '' should be 'null' instead. I will send a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danmarshall , OK, then will wait PR instead of writing it up by my own.
Cool fix @kwonoj - however I am seeing a specific issue when server replies with HTTP 204 - No Content. When this code executes:
If |
@danmarshall Thanks for suggestion. |
Not sure if this is the appropriate place to put this, but I Just encountered this bug in a different place: when the server returns JSON in a 4xx / 5xx response.
Here's what my code looks like (action$, store) => action$ .ofType(MY_TYPE) .switchMap( action => Observable.ajax({ url: 'myUrl', method: 'POST', body: action.options, responseType: 'json', }) .map(...) .catch(error => Observable.of({type: FETCH_FAILURE, errorResponse: error.xhr.response})), ) |
@maximusdominus Were you able to resolve this issue? I'm facing the same issue. UPDATE: easiest workaround is to detect (typeof error.xhr.response === 'string') and use JSON.parse accordingly. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description:
This PR updates internal behavior of AjaxObservable for IE specific by IE doesn't support json as response type, parse response as json object internally.
Related issue (if exists):
closes #1381