errors in .ajax are not propagated to .catch

[cdaringe]

hello,

it seems that errors may not be being properly handled in the ajax operator.

RxJS version:

5.3.0

Code to reproduce:

// index.js (run in node, after installing rxjs, nock, & xmlhttprequest)
var nock = require('nock')
var Observable = require('rxjs').Observable
global.window = global
global.XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest

nock('http://example.com').get('/test/1').reply(200, '!!INVALID-JSON!!') // :eyes:

var obs = Observable
.from([1])
.switchMap(action => {
  return Observable
  .from([2])
  .ajax({ url: 'http://example.com/test/1', method: 'GET' }) // does not work per expectation
  // .map(({ response }) => { throw new Error('bananas') }) // works per expectation
  .catch(err => {
    console.error(err.message) // never reached when JSON wasn't parseable from ajax request
    process.exit()
  })
})

obs.subscribe(arg => console.log('bananas'))

Expected behavior:

.catch entered

Actual behavior:

.catch not entered

Additional information:

[kwonoj]

It seems JSON.parse exception is not propagated into error handler somehow?

I'm feeling maybe time to push forward this umbrella discussion (https://github.com/ReactiveX/rxjs-core-notes/blob/master/2017-03/march-27.md#splitting-dom-observables-off), which'll possibly resolve lot of issues with ajax implementation (and probably ng's http module is more battle-tested).

[benlesh]

Honestly, I think I'd like to unify the work between Angular's Http service and our Ajax implementation into something more framework agnostic. Both solutions have their issues, and I think it would be better to focus on one.

[staltz]

How about using something out there like Axios or Superagent? Both are XHR-based and quite reliable. It's a huge project to start an ajax library from scratch, and get every corner case right.

[benlesh]

The Angular Http Service is XHR based and quite reliable and is already an Observable. Also it's maintained by people sitting within 300 ft of me. Seems like a win.

[jayphelps]

We just ran into this and can confirm. AFAICT this will only be hit in cases where you're using a non-spec compliant XMLHttpRequest implementation. e.g. https://www.npmjs.com/package/xmlhttprequest but NOT with https://www.npmjs.com/package/xhr2 or a browser's native XHR. Here is where the spec defines this behavior: https://xhr.spec.whatwg.org/#json-response (same behavior in xhr1 spec)

When a compliant XHR responseType = 'json' it attempts to parse the JSON (usually on a separate thread, non-blocking) and if it fails it simply sets the xhr.response = null and silently swallows any parsing errors. This is just how it is as a tradeoff of parsing off thread (there would be no JS stack trace to give as well).

That isn't to say that rxjs shouldn't still try and handle this--I am personally in the camp of "if an exception could be thrown, we should catch and propagate it". Just an FYI in my research into this.

[lkarmelo]

We also encountered this bug(?). Together with that #1589 (comment) it silently crashes our frontend app when response is html instead of expected json. It happends on 502 error, because nginx i don't have control over sends error message as html page. In IE 11 response type is empty string and this code

this.responseType = xhr.responseType || request.responseType;
this.response = parseXhrResponse(this.responseType, xhr);

//later

switch (responseType) {
        case 'json':
            if ('response' in xhr) {
                //IE does not support json as responseType, parse it internally
                return xhr.responseType ? xhr.response : JSON.parse(xhr.response || xhr.responseText || 'null');
            }
            else {
                return JSON.parse(xhr.responseText || 'null');
            }
        case 'xml':
            return xhr.responseXML;
        case 'text':
        default:
            return ('response' in xhr) ? xhr.response : xhr.responseText;
    }

assumes it's a json.