search

martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 77 forks source link

Add back old "reject on failure" behavior for HTTP #309

Closed taion closed 9 years ago

taion commented 9 years ago

For martyjs/marty#308

The other changes to the test were due to some sloppiness with response codes that previously got through because we weren't rejecting on remote failures. Also, Sinon's fake server doesn't seem to work with native fetch (because it replaces XHR, which is only used by the polyfill), so need to match what's on the mock server as well.

I think the right thing to do is to just call into https://github.com/mzabriskie/axios instead of having this code in Marty, but it's probably best to have the old behavior back for the time being.

jhollingworth commented 9 years ago

I don't think we should re-add automatically throwing errors. I remember people raising issues about it in the past and in general I want to shy away from marty doing too much for you.

I would quite like to move to axios and agree its annoying you cannot use sinon fake server however I have a couple of concerns:

  1. Interceptors only work on the request/response data rather than the whole request/response which would mean we couldn't make cross cutting changes to the request/response
  2. Their API is very different from fetch's which would mean a painful migration path

Right now I don't see the pain in doing this migration is worth the benefit of making the move but happy to be convinced otherwise :smile:

jhollingworth commented 9 years ago

Thinking about it, i'd be happy to accept this PR if you remove this line. That way people can easily re-add that behaviour but its not a default

taion commented 9 years ago

I'm not a huge fan of this behavior either. The problem is

1) This change wasn't documented and looked to be unintentional, because the test still covered the behavior (but both the behavior and the test were broken) 2) The documentation still implies that this is the case (because the queries examples all look for promise rejection, and the HTTP state source example doesn't check req.ok) 3) Most users still expect that things behave this way (mostly due to 1). I personally got bitten by this, but didn't look into it closely until just now.

I think we should give serious consideration to dropping this feature and documenting and announcing this for v10 or v11, but I do think we should restore the old behavior for v9, just to avoid surprising people.

taion commented 9 years ago

My point is that, while we should get rid of this feature, we should do so correctly, not silently via a broken test case.

jhollingworth commented 9 years ago

I completely agree this change should have been communicated better however I intentionally removed it for the v0.9 release. Re-introducing the behaviour now would break peoples websites and semantic versioning in my eyes.

taion commented 9 years ago

I see - I assumed that it was removed unintentionally, because the test case is still there: https://github.com/martyjs/marty/blob/3a625f77b542e15f00b44c307a01a7f87cff54d5/test/browser/stateSources/httpStateSourceSpec.js#L182-L206

There's no natural place in v0.9 to drop in compatibility hooks to enable old behavior, right? Maybe the right thing to do would be to backport those from v0.10 and hide it behind one of those.

jhollingworth commented 9 years ago

I've merged this PR but you have to add HttpStateSource.addHook(require('marty/hooks/throwError')) when the application starts to have the legacy behaviour

taion commented 9 years ago

We might want to modify the hook to have a lower priority (or a higher one... at least so it runs predictably relative to other hooks), and maybe bring back the test case, but on closer thought I think I don't want to use this in my code for now, so I'm going to hold off on making any further changes unless other issues come up.