inveniosoftware / react-searchkit

React component library for interacting with a REST API.
https://inveniosoftware.github.io/react-searchkit/
MIT License
78 stars 40 forks source link

InvenioSearchApi: accidental use of both "baseURL" and "url" config #88

Closed slint closed 4 years ago

slint commented 4 years ago

axios supports both url and baseURL parameters, setting an absolute url though (i.e. url: "https://localhost:5000/api/communities", cancels baseURL.

Setting baseURL: config.url and then expanding the config via ...config, is the equivalent of:

const axiosConfig = {
  baseURL: config.url,
  url: config.url,
};

When a relative url is used (e.g. url: "/api/communities"), this causes the final requests to be made to baseURL + url = "/api/communities/api/communities".

slint commented 4 years ago

After some discussion with @zzacharo (CC @ntarocco), it is actually possible to have the desired behaviour by passing both keys to the InvenioSearchApi class:

const searchApi = new InvenioSearchApi({
  baseURL: "https://localhost:5000/api/communities",
  url: "",
})

In general though, the discussion should be about whether to provide a wrapper around the axios config (which is the case now because of the special use of the url parameter), or to define what kind of arguments are specific to the class and have the rest of them be directly passed to the axios config.

ntarocco commented 4 years ago

In the initial discussions when designing react-searchkit, we did not want to have a wrapper of axios and define a specific subset of axios config to pass. That url/baseUrl has been changed afterwards. Honestly I find baseUrl and url very confusing in axios: one is the baseUrl indeed, but the other one is for me more a path or a pathname. I would come back to the initial implementation and allow to pass config directly to axios without this manipulation of url/baseUrl introduced later on, so it is clear that you have to inject axios config.