Skip to content
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

add more things to success response, like status, header, maybe even allow it to be extended #320

Closed
klis87 opened this issue Mar 22, 2020 · 12 comments

Comments

@klis87
Copy link
Owner

klis87 commented Mar 22, 2020

For now success responses have only data, we could add more stuff, make it configurable, not sure it is needed though.

@samueledirito
Copy link

This is really critical.
It's mandatory to receive full response.

To provide a silly example, like when you need to read Authorization header to complete a login!

Just joking, but: Come on, don't make me patch it locally :D :D

@klis87
Copy link
Owner Author

klis87 commented Apr 2, 2020

@samueledirito I was afraid that header is a must. Usually people would return token in data, not response header, but indeed this sometimes happens.

It's mandatory to receive full response.

What response keys would you consider as a must? Apart from data

You might wonder why there is response.data only for now. In previous versions of this library actually I returned all response in sendRequest for instance or promisified request action. But I stored only data in reducer. It has to be consistent everywhere though in order to make SSR and cache work, as those just take response from reducer instead of making AJAX call. In reducer there is only data that's why I took 1 step back and reducer response only to data. Also, response has to be serializable, otherwise SSR wont work and future persistent cache wont work either.

So the questions are:

  1. what response keys are worth storing in reducer
  2. should we make this configurable or just drivers should by default include more than data
  3. whether we should store some stuff for mutations, as mutations dont have data stored. In theory we could return more than data for mutation and not store those as cache and ssr is only for queries anyway

@samueledirito
Copy link

samueledirito commented Apr 2, 2020

As you said, I think you're introducing a breaking change (removing complete response) due that before you provided it.
You haven't already extracted any interfaces between different drivers, so changing only redux-saga-requests-axios so that returns everything shouldn't be a problem (but a solution, really) for anyone.

In addition (my fault, I don't know well how SSR works), are you really sure that axios driver and SSR are related? Maybe it's just a potential problem that anyway will occur lesser than using it on CSR.

@klis87
Copy link
Owner Author

klis87 commented Apr 2, 2020

the problem is consistency, I want response to be the same read from reducer and from dispatched request and when reading from cache. Driver could return more than data but I want response to have the same shape everywhere, and no matter whether using cache and ssr or not, otherwise people will compain and bugs will occur. Thats why I need to have my questions answered before I think how to solve it.

@samueledirito
Copy link

Ok, I catch your point.
To resolve this, I think it could be good if Authorization header should be provided at less.

I can't imagine someone who uses your package (that is amazing!!!) and talks with a backend via custom headers :D It's really umpleasant :( :(

@klis87
Copy link
Owner Author

klis87 commented Apr 2, 2020

we need to be generic and flexible, so if headers, all headers should be there, no matter whether Authorization or not. Anything else apart from headers?

@samueledirito
Copy link

Not really, you'll save my day if you could open an alpha with headers only :D

@klis87
Copy link
Owner Author

klis87 commented Apr 2, 2020

do u use axios?

@samueledirito
Copy link

Yes!
If you need any help, we can chat via hangouts

@klis87
Copy link
Owner Author

klis87 commented Apr 3, 2020

@samueledirito for now you can use axios interceptor, sth like:

axios.interceptors.response.use(response => {
  if (!response.headers.Authorization) {
    return response;
  }

  // assuming that response.data is object, if not, adjust below
  return {
    ...response,
    data: { ...response.data, token: response.headers.Authorization }
  };
});

This will just add header information to data itself.

@zeraien
Copy link

zeraien commented Apr 29, 2020

I'd have to say that I have not felt the need to use response headers per se, but sometimes I want to know status codes returned by the server, like if it's a 302 or something, you don't want that to be a real error - just something that gets ignored or reverted, so maybe you just wanna ignore that error... Not sure it's related, but it feels related.

@klis87
Copy link
Owner Author

klis87 commented Jul 20, 2020

Now axios and fetch drivers support status and headers in response next to data

@klis87 klis87 closed this as completed Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants