Help me decipher the smell of this code | TypeScript Community | Page 1

lone wharf Aug 11, 2024, 9:31 PM

#

So I am looking at some code where they made an HttpClient to use for an API, I can see there's a smell extending the Axios class without using its constructor, seemingly creating one extra Axios instance that doesn't need to exist, I can see some more memory usage, but other than that, I can't explain why it's so smelly except my intuition, here's the code :

export class BaseApiOriginal extends Axios {
  instance: AxiosInstance

  constructor() {
    super()

    this.instance = axios.create({
      baseURL: baseURL
    })
  }

  static async build() {

    const baseApi = new BaseApiOriginal()

    let headers: Partial<AxiosHeaders> = {
      'Content-Type': 'application/json'
    }
    console.log(baseApi)
    baseApi.instance.defaults.headers.common = { ...headers }

    baseApi.instance.interceptors.request.use(
      (request) => request,
      (error) => {
        return Promise.reject(error)
      }
    )

    baseApi.instance.interceptors.response.use(
      (response) => response,
      async (error) => {
        await Promise.reject(error)
      }
    )

    return baseApi
  }


  // Axios Super
  static async getInstance() {
    const instance = await BaseApiOriginal.build()
    return instance.instance
  }

  static async getUri(config?: AxiosRequestConfig): Promise<string> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.getUri(config)
  }

  static async request<T = any, R = AxiosResponse<T>, D = any>(config: AxiosRequestConfig<D>): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.request<T, R, D>(config)
  }

  static async get<T = any, R = AxiosResponse<T>, D = any>(url: string, config?: AxiosRequestConfig<D>): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.get<T, R, D>(url, config)
  }

  static async delete<T = any, R = AxiosResponse<T>, D = any>(url: string, config?: AxiosRequestConfig<D>): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.delete<T, R, D>(url, config)
  }

  static async head<T = any, R = AxiosResponse<T>, D = any>(url: string, config?: AxiosRequestConfig<D>): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.head<T, R, D>(url, config)
  }

  static async options<T = any, R = AxiosResponse<T>, D = any>(
    url: string,
    config?: AxiosRequestConfig<D>
  ): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.options<T, R, D>(url, config)
  }

  static async post<T = any, R = AxiosResponse<T>, D = any>(
    url: string,
    data?: D,
    config?: AxiosRequestConfig<D>
  ): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.post<T, R, D>(url, data, config)
  }

  static async put<T = any, R = AxiosResponse<T>, D = any>(
    url: string,
    data?: D,
    config?: AxiosRequestConfig<D>
  ): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.put<T, R, D>(url, data, config)
  }

  static async patch<T = any, R = AxiosResponse<T>, D = any>(
    url: string,
    data?: D,
    config?: AxiosRequestConfig<D>
  ): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.patch<T, R, D>(url, data, config)
  }

  static async postForm<T = any, R = AxiosResponse<T>, D = any>(
    url: string,
    data?: D,
    config?: AxiosRequestConfig<D>
  ): Promise<R> {
    const instance = await BaseApiOriginal.build()
    return instance.instance.postForm<T, R, D>(url, data, config)
  }
}

real torrent Aug 11, 2024, 9:36 PM

#

extending the Axios class without using its constructor
wdym by that? the superconstructor is called

hoary helm Aug 11, 2024, 9:49 PM

#

Oh yeah this looks awful
If you call BaseApiOriginal.get, it calls BaseApiOriginal.build() which creates a BaseApiOriginal, which creates a sub instance with axios.create. But I don't think that instance is actually used, it usually uses the original BaseApiOrginal instance

#

You can also do const api = new BaseApiOriginal and just have an axios with an unused sub instance

hoary helm Aug 11, 2024, 9:51 PM

#

hoary helm Oh yeah this looks awful If you call `BaseApiOriginal.get`, it calls `BaseApiOri...

Actually it returns a BaseApiOrginal but you have to do api.instance to use the customized instance. But if you use api.getInstance this doesn't get the instance, it creates a new instance with a new sub instance.

#

this whole thing should probably just be a function that calls axios.create, customises it, and returns that

lone wharf Aug 11, 2024, 9:52 PM

#

real torrent > extending the Axios class without using its constructor wdym by that? the supe...

it is called, but it's not used to construct the axios object, which is what this.instance = axios.create({}) does. If I am not mistaken, you have to use super() to use a class children, but usually you'll use that to instanciate with the parents option

lone wharf Aug 11, 2024, 9:56 PM

#

hoary helm Actually it returns a BaseApiOrginal but you have to do `api.instance` to use th...

Yeah it's a bit weird

lone wharf Aug 11, 2024, 10:00 PM

#

hoary helm this whole thing should probably just be a function that calls axios.create, cus...

I tried following that philosophy because why not, but to me it felt like it was worthless extending axios without using the axios object at all. So I just did it to have a static object that you can use to make api calls, but I agree with you, it could just be a function that handles the API calls

hoary helm Aug 11, 2024, 10:00 PM

#

If you have a static object to make calls, don't extend Axios, because that's blurring 2 things together

#

and don't call .build everytime you use it

#

Instead call .getInstance, and have getInstance build a new instance only if there is not one already

lone wharf Aug 11, 2024, 10:01 PM

#

Here's the way I did it :

export class BaseApiWithoutSuper {
  static async build() {
    const client = axios.create({
      baseURL: baseURL
    })

    const accessToken = getAccessToken()

    let headers: Partial<AxiosHeaders> = {
      'Content-Type': 'application/json'
    }
    console.log(client)
    client.defaults.headers.common = { ...headers }
    if (accessToken) client.defaults.headers.common.Authorization = `Bearer ${accessToken}`

    client.interceptors.request.use(
      (request) => request,
      (error) => Promise.reject(error)
    )

    client.interceptors.response.use(
      (response) => response,
      async (error) => {
        if (error && error.code === AxiosError.ERR_CANCELED) return Promise.reject(error)
        if (error.response && error.response.status === HttpStatusCode.Unauthorized) {
          await BaseApiWithoutSuper.refreshTokenAPI()
        }
        await Promise.reject(error)
      }
    )

    return client
  }

  static async getInstance() {
    const instance = await BaseApiWithoutSuper.build()
    return instance
  }
// all the axios methods defined after in the same way 
  static async request<T = any, R = AxiosResponse<T>, D = any>(config: AxiosRequestConfig<D>): Promise<R> {
    const instance = await BaseApiWithoutSuper.getInstance()
    return instance.request<T, R, D>(config)
  }

#

To me this felt more natural

hoary helm Aug 11, 2024, 10:02 PM

#

Every time you do BaseApiWithoutSuper.request you are creating a new instance

lone wharf Aug 11, 2024, 10:02 PM

#

Is there a way to avoid that with static classes ?

hoary helm Aug 11, 2024, 10:02 PM

#

yes

lone wharf Aug 11, 2024, 10:03 PM

#

oh yeah using get instance like a singleton pattern

#

is that what you meant ?

humble bronzeBOT Aug 11, 2024, 10:04 PM

#

sandiford#0

Playground Link

Preview:```ts
export class BaseApiWithoutSuper {
instance: Axios | null = null

static async build() {
const client = axios.create({
baseURL: baseURL,
})

const accessToken = getAccessToken()

let headers: Partial<AxiosHeaders> = {
  "Content-Type": "application/json",
}
console.log(client)
client.defaults.headers.common = {...headers}
if (accessToken)
  client.defaul

...```

hoary helm Aug 11, 2024, 10:04 PM

#

yep

humble bronzeBOT Aug 11, 2024, 10:04 PM

#

sandiford#0

Playground Link

Preview:```ts
export class BaseApiWithoutSuper {
instance: Axios | null = null

static async build() {
const client = axios.create({
baseURL: baseURL,
})

const accessToken = getAccessToken()

let headers: Partial<AxiosHeaders> = {
  "Content-Type": "application/json",
}
console.log(client)
client.defaults.headers.common = {...headers}
if (accessToken)

...```

hoary helm Aug 11, 2024, 10:05 PM

#

Or this way, same result

#

If you want a new instance then there's no point in have getInstance + build, that can be one function

lone wharf Aug 11, 2024, 10:13 PM

#

Thanks 😄 I am looking at it, I like the 2nd way the syntax is elegant. Was just playing with the code a bit => you need to use static instance to use a property in static classes

hoary helm Aug 11, 2024, 10:13 PM

#

yeah you're right

#

There was a lot of type errors, so I couldn't see mistakes easily

lone wharf Aug 11, 2024, 10:14 PM

#

But I actually didn't think about using a singleton pattern on the getinstance and just use that, should be the "best" way of doing it using a static class, I am not very good with Axios so i don't know what's best practice for requests

hoary helm Aug 11, 2024, 10:14 PM

#

I dunno

#

most people have moved to fetch though

lone wharf Aug 11, 2024, 10:14 PM

#

Yeah honestly I would move to fetch if it was my code lmao

hoary helm Aug 11, 2024, 10:14 PM

#

Because it is built into the web and node

#

I would just use fetch/axios raw without anything special

#

unless you need to do some processing on all requests/responses, then that's good

lone wharf Aug 11, 2024, 10:15 PM

#

even if Axios makes a lot of abstraction for you, I feel like 90% of needs concerning APIs Axios doesn't make you gain that much time

hoary helm Aug 11, 2024, 10:15 PM

#

I prefer Axios. It's just not really needed when you have fetch built in

#

Especially if you are wrapping it somehow

lone wharf Aug 11, 2024, 10:17 PM

#

Yeah I agree, but even if you're wrapping it, building the httpclient from scratch seems a bit overkill for a project, so i understand them using axios as a base for their http client

#

what I didn't understand was why the need to extend + create useless instances

#

It was cool having your opinion on the code @hoary helm 🙂

#

!resolved

hoary helm Aug 11, 2024, 10:26 PM

#

I don't think they actually used Axios, it didn't seem like they needed to extend it

#

Except in instance, but they could have made instance an Axios instance instead of self

lone wharf Aug 11, 2024, 10:28 PM

#

hoary helm I don't think they actually used Axios, it didn't seem like they needed to exten...

I think so aswell, which is why my version didn't extend it, since they create it anyway. But they used it in their .instance which is what I refer as "the base" for their http client, it's just it had some weird Axios.defaults & Axios.interceptors wrapper around it lol

#

But your version is better since it uses instance as a singleton

#

I mean at least design pattern-wise idk if it's better to have a single or multiple instances of axios when you do request, it feels like it doesn't matter that much

#

except in memory usage, but we're talking about 100b

hoary helm Aug 11, 2024, 10:30 PM

#

   static async build() {

    const baseApi = new BaseApiOriginal()

    let headers: Partial<AxiosHeaders> = {
      'Content-Type': 'application/json'
    }
    console.log(baseApi)
    baseApi.instance.defaults.headers.common = { ...headers }

    baseApi.instance.interceptors.request.use(
      (request) => request,
      (error) => {
        return Promise.reject(error)
      }
    )

    baseApi.instance.interceptors.response.use(
      (response) => response,
      async (error) => {
        await Promise.reject(error)
      }
    )

    return baseApi
  }

This could be

   static async build() {

    const axios = axios.create({
      baseURL: baseURL
    })

    let headers: Partial<AxiosHeaders> = {
      'Content-Type': 'application/json'
    }

    axios.defaults.headers.common = { ...headers }

    axios.interceptors.request.use(
      (request) => request,
      (error) => {
        return Promise.reject(error)
      }
    )

    axios.interceptors.response.use(
      (response) => response,
      async (error) => {
        await Promise.reject(error)
      }
    )

    return axios
  }

lone wharf Aug 11, 2024, 10:31 PM

#

yup

hoary helm Aug 11, 2024, 10:31 PM

#

lone wharf I mean at least design pattern-wise idk if it's better to have a single or multi...

I've always just use axios.post. I think axios.create is mostly for when you want to use some settings repeatedly.

#

axios probably doesn't use much memory. Each active request will use something regardless.

lone wharf Aug 11, 2024, 10:32 PM

#

hoary helm ```ts static async build() { const baseApi = new BaseApiOriginal() ...

when i started digging that code, this is the first thing i did, stop creating a new object and just create the axios instance there instead of in this.instance

hoary helm Aug 11, 2024, 10:32 PM

#

Probably by having axios and customizedAxios you're just slightly increasing the overhead

lone wharf Aug 11, 2024, 10:33 PM

#

yeah honestly i would love to know how to calculate the impact on performance of those overhead, would make testing performance so much easier

hoary helm Aug 11, 2024, 10:39 PM

#

You can run a node process and create 1000 axios instances to see mem consumption

#Help me decipher the smell of this code