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

Could you replace "axios" with fetch? #898

Open
jimmywarting opened this issue Jan 20, 2023 · 16 comments
Open

Could you replace "axios" with fetch? #898

jimmywarting opened this issue Jan 20, 2023 · 16 comments
Labels
status: help wanted requesting help from the community type: twilio enhancement feature request on Twilio's roadmap

Comments

@jimmywarting
Copy link

... as part of #728 ?

let users bring in there own fetch implementation.

@shrutiburman shrutiburman added the type: twilio enhancement feature request on Twilio's roadmap label Jan 24, 2023
@beebzz beebzz added the status: help wanted requesting help from the community label Jan 26, 2023
@beebzz
Copy link

beebzz commented Jan 26, 2023

Hey @jimmywarting, we'll add this to our internal backlog to be prioritized. +1s/reactions on this issue (and comments) and PRs will bump it higher up the backlog.

@mariomui
Copy link

@jimmywarting I don't see the point of getting rid of axios.
Axios client has other stuff like httpsAgent and a dependency of Request which has a large surface. I guess you could map a adapter interface so you could use undici but...there are prolly bigger fish to fry.

It's also super small as a dependency.
https://github.com/mariomui/twilio-node/tree/smaller-build. <---I shaved off 100kb to make it 1.8mb but someone else is gonna need to test this a lambda project handy.

In my opinion:

  • Deprecate the pattern of : exports = initializer
    • Basically amounts to -> module.exports = initialzer ---> Which can be done by babel.
    • also -> we can get rid of the plugin "babel-plugin-replace-ts-export-assignment"
  • use tsc to generate the types only, export them into dist/interfaces/
  • use babel to transpile typescript down to js. (performance)
  • convert existing code to get to ESM status.
  • (wait for someone higher up to spend money to rearchitect 4.0.0 to a monorepos with multiple packages)

@jimmywarting
Copy link
Author

jimmywarting commented Jan 31, 2023

axios have many things i do not like about it

  • Dose not work in service workers, Deno or bun.js (cuz it use either xhr or node:http(s)
  • responseType = arrayBuffer is inconsistent in different env.
  • depends on the legacy form-data package
  • Don't support streams in browsers
    • b/c it depends on the legacy XMLHttpRequest
  • the stream api isn't based on whatwg stream
  • Even though i used { responseType: 'text' } on a request it still parsed it as json b/c it included application/json content-type headers
  • dose thing with node:buffer instead of using Uint8array
  • dose not support spec compatible FormData or Blob on NodeJS
  • and no i don't think axios is a "small as a dependency"
  • and you can ofc use Agent with fetch as well if you want that.

@bnb
Copy link
Member

bnb commented Feb 2, 2023

At the end of April, Node.js v14 will be EOL and v16 will be the minimum supported version of Node.js. unidici-fetch fully supports Node.js v16 (I believe some features are missing from v14 that undici-fetch uses). Undici is an official project under the Node.js project, so it will be very will supported within Node.js, and problems will get resolved rapidly as they come up. Undici also generally has a pretty hardcore focus on performance, which will very likely benefit us here.

undici-fetch is directly included in Node.js v18 as Node.js's official fetch implementation, meaning that next year a third-party dependency could be entirely dropped since Node.js will support fetch directly, natively, on all supported release lines.

Outside of adopting undici-fetch, I do +1 allowing users to bring their own fetch implementation if they don't want to use the one we provide. There is already support for a Custom HTTP Client (thank you @beebzz for pointing this out!), so I'm not sure how different "custom fetch" (request in the OP) and "custom HTTP client" are in practice - they're not exactly compatible, but they're not totally incompatible. This request is basically asking for twilio-node to switch to using fetch rather than other HTTP request methods, which is important to note.

@aaronhuggins-carewell
Copy link
Contributor

Absolutely would love to see fetch used instead of axios.

@nileshtrivedi
Copy link

nileshtrivedi commented Feb 16, 2023

+1

IF this makes twilio usable in Deno environments instead of NodeJS, I'd love to have it too.

@jimmywarting
Copy link
Author

Same could be said for Bun.js

@nileshtrivedi
Copy link

@UrielCh
Copy link

UrielCh commented May 9, 2023

There's currently just one Twilio-related package listed on Deno.land which hasn't seen any activity for almost 2 years now.

It won't do much with a single file having less than 50 lines.

by the way, this project is only exported as CJS.

@sarankumar-ns
Copy link

+1

@paperless
Copy link

Also had issues with this. Using bun and the promise was never resolving for messages.create

@riley
Copy link

riley commented Feb 12, 2024

+1 Unless I'm missing something, twilio-node doesn't work in my Electron app at all (node.js context). I would like to use twilio as a platform, but this is obviously a dealbreaker.

twilio gives this error when trying to run the Hello World code after signing up.

client.calls.create({
      url: "http://demo.twilio.com/docs/voice.xml",
      to: "<my_number>",
      from: "<my_twilio_number>",
})
 .then(call => console.log(call.sid));

UnhandledPromiseRejectionWarning: AxiosError: There is no suitable adapter to dispatch the request since :
- adapter xhr is not supported by the environment
- adapter http is not available in the build

@OrRosenblatt
Copy link

+1

@ibash
Copy link

ibash commented Apr 24, 2024

except for typescript nonsense, this seems to work:

set a custom request client but with twilio's type

  import twilio, { type RequestClient as TwilioRequestClient } from 'twilio'

const  client = twilio(
    process.env.TWILIO_ACCOUNT_SID!,
    process.env.TWILIO_AUTH_TOKEN!,
    {
      httpClient: new RequestClient() as TwilioRequestClient
    }
  )

And a custom request client implementation:

// axios (which twilio uses) breaks in bun, so override with fetch by having our
// own RequestClient implementation

// couldn't figure out out how to import the types of RequestOptions and Response
// from the twilio sdk, so copied it
type HttpMethod = 'get' | 'post' | 'put' | 'patch' | 'delete'
interface Headers {
  [header: string]: string
}

interface RequestOptions<TData = any, TParams = object> {
  method: HttpMethod
  uri: string
  username?: string
  password?: string
  headers?: Headers
  params?: TParams
  data?: TData
  timeout?: number
  allowRedirects?: boolean
  forever?: boolean
  logLevel?: string
}

class Response<TPayload> {
  constructor(
    public statusCode: number,
    public body: TPayload,
    public headers: any
  ) {}

  toString(): string {
    return 'HTTP ' + this.statusCode + ' ' + this.body
  }
}

export default class RequestClient {

  async request<TData>(opts: RequestOptions<TData>): Promise<Response<TData>> {
    const { uri, method, username, password, data, ...rest } = opts

    let headers = opts.headers || {}

    // from twilio sdk
    if (opts.username && opts.password) {
      const auth = Buffer.from(opts.username + ':' + opts.password).toString(
        'base64'
      )
      headers.Authorization = 'Basic ' + auth
    }

    let body: URLSearchParams | string | null = null
    if (
      headers['Content-Type'] === 'application/x-www-form-urlencoded' &&
      data
    ) {
      // TODO(ibash) typescript nonsense
      // Type 'import("url").URLSearchParams' is not assignable to type 'URLSearchParams'.
      body = new URLSearchParams(data) as any
    } else if (headers['Content-Type'] === 'application/json' && data) {
      body = JSON.stringify(data)
    }

    const response = await fetch(uri, {
      // TODO(ibash) uppercase method?
      method,
      headers: headers,
      body: body
    })

    // TODO(ibash) handle other response types
    let responseBody: TData | null = null
    const contentType = response.headers.get('Content-Type')
    if (contentType?.includes('application/json')) {
      responseBody = (await response.json()) as TData
    } else {
      throw new Error(`Unhandled response type: ${contentType}`)
    }

    return new Response(
      response.status,
      responseBody as TData,
      response.headers
    )
  }
}

@mfulton26
Copy link

ky can also be used to make working with fetch() easier and handling common patterns (hooks, errors, etc.)

@tawanaj
Copy link

tawanaj commented Jun 7, 2024

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests