twilio / twilio-node

Node.js helper library
MIT License
1.37k stars 497 forks source link

Could you replace "axios" with fetch? #898

Open jimmywarting opened 1 year ago

jimmywarting commented 1 year ago

... as part of #728 ?

let users bring in there own fetch implementation.

beebzz commented 1 year ago

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 commented 1 year ago

@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:

jimmywarting commented 1 year ago

axios have many things i do not like about it

bnb commented 1 year ago

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 commented 1 year ago

Absolutely would love to see fetch used instead of axios.

nileshtrivedi commented 1 year ago

+1

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

jimmywarting commented 1 year ago

Same could be said for Bun.js

nileshtrivedi commented 1 year ago

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

UrielCh commented 1 year ago

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 commented 6 months ago

+1

paperless commented 6 months ago

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

riley commented 5 months ago

+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 commented 3 months ago

+1

ibash commented 2 months ago

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 commented 1 month ago

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

tawanaj commented 1 month ago

+1