Global `onSuccess` interceptor fires AFTER action `getData`?

[zeraien]

I noticed that if you put onSuccess interceptor in handleRequests, it is always fired after the getData method of each individual action. But consider this use case:

Many API's include a "success" key in their responses, rather than using HTTP error codes (Slack for example uses an ok key), so you might want to have a central place to process the API response and look for ok or success key, and remove them from the equation... For example.

Consider this response:

{
   "ok": true,
   "objects": []
}

So in your FETCH action you might use getData:

getData: response => response['objects']

But this also means that your onSuccess call no longer sees the ok key at all, you just see the result of the getData call...

handleRequests({
        onSuccess: (response, action, store) => {
            //response.data['ok'] is no longer there, it's been processed by `getData`.
            return response;
        },
       ...
})

I'm not sure if this is desired behavior or a bug, but I somehow feel that central interceptors should get the fresh data first and the actions get the data last... What do you think?

I am aware that we might just do processing of the response in the onSuccess interceptor, but that will create inconsistent results of the actions getData call sometimes touches the response data. The best case scenario is that the global interceptor homogenizes the response so that all actions always know what to expect if they need to getData...

I suppose the easiest solution to this might be to have an onResponse global interceptor? Since we also have no way of labeling a request as an error from onSuccess...

[klis87]

Here you can see the response flow https://github.com/klis87/redux-requests/blob/master/packages/redux-requests/src/middleware/create-send-requests-middleware.js#L294

I have plans to make a chart showing all of this, and what happens when caching, ssr rehydration and so on

You are right, getData is called before and really it probably should be different.

Actually I am thinking about deprecating getData, you can achieve the very same thing with meta.onSuccess, so getData is redundant and duplicating onSuccess in API.

Regarding onResponse I am going to add it, so it will be possible to change success into error.

For now you could achieve onResponse with just a driver, whether you would copy the one you use and update it to your need, or wrap a driver like axiosDriver and add chained then to do any transformation you want.

You touch actually a very difficult part of design in this lib, there are several issues I will need to tackle which are not ideal now: 1) the order like you describe 2) onResponse 3) what about calling some of interceptors for cached queries? those already have data processed 4) what about dispatching requests in interceptors? we have issues like https://redux-requests.klisiczynski.com/docs/tutorial/6-interceptors#metasilent-and-metarunon , you need to be careful sometimes no to have some interceptors run twice for the same request, or to avoid infitite loops. There are ways to do this like shown in the docs, but I have plans to make this automatic 5) probably I will change return type in interceptors to be like in dispatch, so instead of throwing, you would return { error }, then it would be possible for example to do thing like return dispatch(anotherRequestAction) instead of checking error key and throwing error or returning response

[zeraien]

Every time you think you've made progress, some code monkey pops up and makes your life miserable ;-)

But anyway, thanks for the driver tip, I'll think about it, either go with driver or use http codes, i haven't quite decided yet on the final API so it's still easy to change. Given how many big API's don't use HTTP codes for their errors, it might make sense to go with the error key, but at the same time, http errors are a tried and true method... Great to hear you'll be adding onResponse.

getData Interesting point about getData, however, it might not be a bad idea to keep it, with getData there is no ambiguity as to it's function, as long as the flow is clear and all that, while onSuccess is a more general method and can be used for more stuff... There are merits to both, some libraries do have special methods just to massage response data, and it can even be argued that onSuccess (et.al) usually does not return data, it feels more like a side-effect trigger... :-P In any event, I think getData should stay for now as long as it doesn't cause too much legacy maintenance.

Interceptors As for interceptors for cached queries, well, that's also interesting, I think that should always receive the original data, but maybe your dilemma is if the interceptors should be called again for cached queries? I think probably interceptors don't need to be called for cached queries... but it's a tough one.

interceptor return type Yeah, returning an error rather than throwing it would be more intuitive. In general I think there is a challenge with error checking. I for example have a pretty wide error checker that checks both HTTP error codes and action.error, and that again presents a dilemma because FSA mandates that action.error is a boolean(?) and here we have actual error data in action.error. But yeah I've found some challenges with this, especially when you submit a form, if the form has validation errors on the server side, in most cases you don't really return a non-200 error code, so getting that to work with this lib is tough because you need to somehow turn your request into an error while it has 200 error code so that you both can present validation errors but also mark the request as failed... it's a challenge.

But in general I think you've made huge progress with this library and it mostly does what it has to do!

[klis87]

Re getData thx for your thoughts, indeed I also have mixed feelings about it

Re interceptors and cached queries, the issue is that they should be called for side effects, but not for data transformation :) And they cannot get raw data, because I store only transformed data. I do it on purpose, because otherwise people doing mutations would need to mutate both raw and transformed data, which would be terrible. Transformed data has to be stored and can be the only one source of truth, otherwise I won't be able to have normalisation, caching, ssr, mutations working together, at least I dont know how otherwise. And caching reuses just data stored, cached queries doesnt have separate information, only that a query is cached and a timeout, which then makes dispatch(cachedRequest) not firing new request but getting data back from store.

I have some stuff to do before, but once I have a design plan for new interceptors, I will ping you so you might add some thoughts and worries before releasing the new version.

[klis87]

@zeraien I though about it and possibly I found the solution for all problems, see design for new interceptors: 1) we would add onResponse interceptor, as well as meta.onResponse, this would be called 1st in the chain, it would have type like ({ success, error, abort }, responseAction) => Promise<{ success, error, abort }>, so in one interceptor you could change error into success and vice versa, you could have logic here like global token refresh, requests retries and so on 2) adding global getData, which would be called after onResponse for successful request (after it meta.getData like now), probably it should be renamed to transformData... 3) onSuccess and onError would stay, but they would return void, they would be called after getData, they would be used only for side effects, no data transformation anymore or recovering from error, for this onResponse should be used 4) duplicated data transformation would be solved now, because onResponse wouldn't be called at all for cached responses, the same for getData, only onSuccess if defined (for side effects), onSuccess would always have data transformed passed, so everything would be consistent 5) for rehydrated queries with SSR, again, onResponse wouldn't be called, as well as getData, onSuccess would be as people might have side effects on frontend only, or only on backend, or both, they could make decision by checking action.meta and act accordingly 6) the tricky part is dispatching request actions from onResponse and returning their response, probably then only onResponse should be called for such actions, the rest not, otherwise all interceptors below onResponse would be called twice. I think this could be handled like store.dispatchRequest(action, { repeat: true }) or sth. The biggest problem here is that this would be needed to be done only in case this new response was returned in onResponse, for side effects or an extra action like refreshToken all interceptors should be called, hence probably we need a config which would allow to handle those 2 scenarios 7) perhaps 6 could be automated somehow, like we could attach attrs to action.meta like getDataCalled: true, so then we could ignore those when appropriate

[klis87]

atually I think that onResponse should have both input and output the same as response from dispatch - https://redux-requests.klisiczynski.com/docs/guides/actions#promisified-dispatches

then it would be possible just to return store.dispatch(requestAction) from onResponse thx to the same structure

[avasuro]

I didn't managed to dive deep into all pros and cons mentioned here (lack of free time), but at a first glance I think it's better to not implement global onSuccess interceptors or whatever handler to separate error and success responses because from my point of view responsibility to properly parse response and throw or not to throw error lies with Driver.

The reason is that, for example, slack API is not truly REST-based because it doesn't follow all of REST rules (at least server can send error responses with status 200, which goes against the documentation), and this means that REST driver (fetch or axios) can't properly handle such responses, and from my point of view to handle that API separate driver should be implemented. This driver should be build on top of some REST driver, and do additional handling of responses with status 200 (if (status === "ok") then return response.object; else throw SomeEror();)

In addition beside standard REST API and those API's that you've mentioned (like slack API etc.) there is many other protocols that never uses any of standard HTTP error responses, and includes information about possible error in response body. Examples of this protocols: JSON-RPC, XML-RPC, SOAP, GraphQL (which is implemented in this library via separate driver, by the way).

So, despite global onSuccess interceptor will be handy for end users of this library (especially for those who doesn't know nothing about API of drivers) I think such interceptor will be a bad design decision. As a possible alternative - may be it will be good to add onResponse interceptor for axios and fetch drivers. This way it will be easier for end users to improve default HTTP behavior based on the needs, e.g. something like this:

import { createDriver as createAxiosDriver } from '@redux-requests/axios';
import axios from 'axios';

const axiosDriver = createAxiosDriver(axios);

const { requestsReducer, requestsMiddleware } = handleRequests({
   driver: {
       rest: axiosDriver,
       // Possible implementation with 'onResponse' interceptor on diver-level:
       slack: createAxiosDriver(axios, {
           onResponse(response) {
               if (response.status === 'ok') return response.object;
               else throw new SlackAPIError(response);
           }
       }),
       // Possible implementation with current architecture (on top of axios driver):
       slack: (requestConfig, requestAction, driverActions) => {
           return axiosDriver(requestConfig, requestAction, driverActions).then(response => {
               if (response.status === 'ok') return response.object;
               else throw new SlackAPIError(response);
           });
       }
   }
})

[klis87]

@avasuro yeah, agreed, examples you mention belong to drivers, and, regarding slack, I am really surprised they did it like this, and indeed status 200 will be treated as a success response. Anyway, the responsibility you mention is about distinguishing errors and successes on protocol level, nice ideas to chain axios driver promise to transform it into another, nice functional technique in practice!

However, there is yet another level, the ap level, like people could want to catch errors with expired token and so on, that shouldn't be part of driver. For this, we need interceptors. Also, one could have a global logic for all responses, no matter what driver used.

The biggest change I am going to make is transforming onSuccess and onError interceptors to support only side effects, and response transforming, or replacing success with error and vice versa will belong to the new onResponse. There are many reasons for this design, but the most important is to allow also replacing success with error (there is issue for that, sometimes we need to "fix" bad API), and also complexity of caching, ssr affecting interceptors, so that you always get the same response, no matter cache hit or not. The current API unfortunately has some holes like that. Also, with the new design, it will be possible to improve logic of "nested requests" - fired from interceptors, which responses would replace native response - like refresh token case - there are many edge cases like infinite loops, double run interceptors etc, which could be automated with the new interceptors API.