Success function invoked in source for any status code returned

[cdownard]

So we updated to 0.9.12 and we're currently seeing the success promise invoked always even though the response.ok is set to false.

We have tried by changing the response status code we return to ones in the 3 and 400 ranges and even though that response object properly has the response.ok set to false, we cannot find a way for it to trigger the failure function instead.

this.post({
      url: '/services/legistar/meeting_statuses',
      body: payload
    }).then(function(response) {
      SomeActionCreators.create(response.body);
    }, function(response) {
      response.json().then(callback, sourceError);
});

The above is a sample selection from our codebase. Is anyone else seeing this?

[taion]

Do you know that this would reject the promise in earlier versions? Fetch itself does not reject the promise for non-"OK" status code.

[cdownard]

It did reject previously. We updated today and we return an array of errors which are coming back in the success function of the promise.

The response.ok is now false inside the success function which doesn't seem to be valid to me.

[taion]

Thanks. What version were you on previously?

[taion]

cc @martyjs/core

I took a quick look. It looks like this commit for v0.9 removed the old behavior of throwing on status >= 400: https://github.com/martyjs/marty/commit/5537f160ce96c4cc137e9076019464e8f90b0181#diff-6359462e10b92745eaee57745617ccf3

The re-write actually broke the test case here: https://github.com/martyjs/marty/blob/master/test/browser/stateSources/httpStateSourceSpec.js#L182-L206 because expectedResponse is never set, so both expectedResponse and actualError are undefined.

I don't know what the right thing to do here is. The underlying fetch API does actually resolve the promise even when the request fails. It'd probably be best to add back the old behavior, but the wrappers around fetch in the HTTP state source are somewhat odd - I feel like it would make more sense to just wrap Axios (https://github.com/mzabriskie/axios) or something instead of having an HTTP client in Marty.

[cdownard]

we previously were on 0.8.12

[cdownard]

:+1: Yay.

[jhollingworth]

@cdownard I've just released Marty v0.9.13 which adds a hook that replicates this behaviour. Its not enabled by default so you need to add HttpStateSource.addHook(require('marty/hooks/throwError')) when the application starts to have

[taion]

:+1: It might be helpful to update the docs to show explicit error handling on the HTTP state source, though.

[jhollingworth]

Yup, added to the list :smile: