CheesecakeLabs / front-end-boilerplate

🍰 CKL Frontend Boilerplate
4 stars 0 forks source link

Feat: usage of axios for requests #28

Closed caiangums closed 4 years ago

caiangums commented 4 years ago

This PR adds the usage of axios for making requests.

Related Issue: #22

This PR contains

beatriz1304 commented 4 years ago

I think that we can simplify this implementation by using the defaults exported from the Axios client. Also, we should implement this with functions instead of classes, because make it`s more readable (my opinion) and simplify a lot the maintenance by reducing the amount of code necessary =).

The implementation bellow was used on the project ApplyPay and served us really well.

import axios from 'axios'

import { getToken } from '_store'

const addToken = config => {
  const token = getToken()
  if (token) {
    // eslint-disable-next-line no-param-reassign
    config.headers.authorization = `Token ${token}`
  }
  return config
}

const createClient = () => {
  const client = axios.create({ baseURL: `${HOST}/api/v1/` })
  client.interceptors.request.use(addToken, Promise.reject)

  return client
}

const instance = createClient()

export default instance

Above is an example with the usage.

import http from '_utils/fetch'

http
  .get('/schools')
  .then(({ data }) => console.log(data))
  .catch(e => console.log(e))

What do you think?

Also, I have other ideas of implementation like using react-hooks by we can discuss this in person.

Let me know your thoughts on this.

beatriz1304 commented 4 years ago

Tagging everyone else in this PR to be aware of this modification, since it'll affect future projects

salatielsql commented 4 years ago

I think that we can simplify this implementation by using the defaults exported from the Axios client. Also, we should implement this with functions instead of classes, because make it`s more readable (my opinion) and simplify a lot the maintenance by reducing the amount of code necessary =).

The implementation bellow was used on the project ApplyPay and served us really well.

import axios from 'axios'

import { getToken } from '_store'

const addToken = config => {
  const token = getToken()
  if (token) {
    // eslint-disable-next-line no-param-reassign
    config.headers.authorization = `Token ${token}`
  }
  return config
}

const createClient = () => {
  const client = axios.create({ baseURL: `${HOST}/api/v1/` })
  client.interceptors.request.use(addToken, Promise.reject)

  return client
}

const instance = createClient()

export default instance

Above is an example with the usage.

import http from '_utils/fetch'

http
  .get('/schools')
  .then(({ data }) => console.log(data))
  .catch(e => console.log(e))

What do you think?

Also, I have other ideas of implementation like using react-hooks by we can discuss this in person.

Let me know your thoughts on this.

I agree with bia and this implementation is way more simple :)

goodsmith commented 4 years ago

@caian-gums reading the axios code base, we could use the axios.create and reuse the instance.

We could think together and make a more simple use of axios.

@beatriz1304 has shown create instance being used in Apply Pay some comments above

caiangums commented 4 years ago

Personally, I prefer the Class approach for this case, creating another layer of abstraction, avoiding wrong calls for the axios API. I agree that this approach can give less simplicity for the first view. But again, this is just my opinion.

I agree with the simplicity presented here too and it seems it's the pick for the team. I'll refactor and ask for a review again! 😄

karranb commented 4 years ago

I like the function approach, but I think we should improve the abstraction. I think at least we should not return the Axios client. That way if at some point we choose to integrate with something besides Axios it would be easier to adapt.

natamvo commented 4 years ago

Sorry, I opened the PR when no comments, but I agree with the class component approach with less code. And in Microgrids we use more than one interfaces, so I think will be a better call using another folder and not "services" for an interface. (ex: interfaces/http, interfaces/socket)

caiangums commented 4 years ago

As suggested by @smaniotto I'll be removing the example code and changing the implementation of the wrapper for axios favoring requests with authentication.

natamvo commented 4 years ago

can we move to 'interfaces/http' folder?