rapidjs / rapid.js

An ORM-like Interface and a Router For Your API Requests
https://rapidjs.drewjbartlett.com
711 stars 45 forks source link

Return data instead of response #29

Closed miljan-aleksic closed 6 years ago

miljan-aleksic commented 6 years ago

Hi and thank you for rapid.js, it's awesome!

When a request is made using rapid, the entire response gets returned. While that may be correct in general I would like to have a way to return only the data.

Because usually the final methods that use my Api doesn't know anything, and should not, about the response. It only expect the data and if there is any error retrieving it, it is the Api Class the one that deals with it.

// current approach
const response = await ApiR.withParams(query).all()
data = response.data

// a not very good solution
const { data } = await ApiR.withParams(query).all()

I'm not sure about the best approach to solve it, perhaps a config or a new method, but would like to hear your thoughts.

drewjbartlett commented 6 years ago

Hmm...this is just how axios handles this. If I only return data then there's no way to retrieve any other request info one might want. I think you may want to have a look at this: https://github.com/axios/axios. Particularly transformResponse

I think you could pass the config in when you define ApiR:

const ApiR = new Rapid({
   apiConfig: {
      transformResponse (response) {
         return response.data;
      }
   }
});

Also have a look at https://rapidjs.io/docs#configuration-apiConfig. That should do the trick for you. Thanks for reaching out about this @miljan-aleksic!

drewjbartlett commented 6 years ago

I also could potentially add an option for the config that's returnData: true or something that'll do this above logic automatically.

RayBenefield commented 6 years ago

Out of curiosity @miljan-aleksic , what makes the destructuring option unsuitable? I personally really like the elegance of the destructuring for { data } as it will be about as "verbose" as passing an additional config option.

miljan-aleksic commented 6 years ago

@RayBenefield, destructuring is awesome, but when trying to abstract requests into API classes, the function calling the API methods should not have to deal with the request object at all. One config option vs dozens of unnecessary destructuring seems better to me.

@drewjbartlett, transformResponse allows transforming the data only, but it still returns the entire response. An interceptor would solve it, although accessing the axios object directly should be avoided.

An option seems good, but if it's only me having this requirement, don't go for it, as I had to drop the idea of using rapid.js. My projects are mostly on the client side and rapid.js focus seems the server side. I will keep an eye on it, though :)

drewjbartlett commented 6 years ago

Sounds good @miljan-aleksic! Thanks for the suggestion nonetheless! I'll likely add the feature :)

mgred commented 6 years ago

I totally agree with @miljan-aleksic and would really love to have the possibility of defining interceptors. Since axios already offers this feature it would be a great fit for rapid.js as well.

drewjbartlett commented 6 years ago

I will have to confirm this @mgred but I believe you can do one of the following:


window.axios.interceptors.request.use(interceptor)

rapid.api.interceptors.request.use(interceptor)
mgred commented 6 years ago

@drewjbartlett good point!

window.axios.interceptors.request.use(interceptor)

This is great, because you can define interceptors across all resources when defining them to the axios instance. Unfortunately this will not work with NodeJS.

Also, when using rapid.js to build custom Resource classes:

import { Rapid } from 'rapid.js';

 class User extends Rapid {

     boot() {
         this.modelName              = 'user';
         this.config.defaultRoute = 'collection';
     }
}

It would be handy to have the possibility to define interceptors, as all other relevant properties per resource, in the boot function.

this.interceptors.response.push((response) => response.data);
drewjbartlett commented 6 years ago

@mgred Why not:


import { Rapid } from 'rapid.js';

 class User extends Rapid {

     boot() {
         this.modelName              = 'user';
         this.config.defaultRoute = 'collection';
         this.api.interceptors.use(interceptor);
     }
}

We can certainly add the option for just interceptor but I think the above should work per resource :)

mgred commented 6 years ago

@drewjbartlett This will not work, since the boot function runs before the initializeAPI function. :( https://github.com/rapidjs/rapid.js/blob/master/src/core/core.js#L42

drewjbartlett commented 6 years ago

Ah good point, @mgred. Well if you want to submit a PR that'd be awesome. I likely won't have time to add this for a few weeks at least :/

mgred commented 6 years ago

Alright, I will care.

mgred commented 6 years ago

@drewjbartlett I implemented a simple version (https://github.com/mgred/rapid.js/tree/mgred/rapid.js-1). Do you have some time for a review and feedback? We could also implement this exposing axios InterceptorManager.

drewjbartlett commented 6 years ago

@mgred could you make a PR so I can easily review the changes?

mgred commented 6 years ago

@drewjbartlett there you go