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

Got client #586

Open
omridevk opened this issue May 16, 2024 · 27 comments
Open

Got client #586

omridevk opened this issue May 16, 2024 · 27 comments
Labels
client Client package related feature 🚀 New feature or request RSVP 👍👎 Explicit request for reactions to gauge interest in resolving this issue

Comments

@omridevk
Copy link

Description

I would like to contribute and add support for Got which is a very popular HTTP lib wrapper of the fetch API.
Is this something you would like me to make a PR about?
I really want to use this library but all of our code is written using Got and we made a lot of wrappers around it, it would be sad not to be able to use it.

@omridevk omridevk added the feature 🚀 New feature or request label May 16, 2024
@mrlubos mrlubos added the client Client package related label May 16, 2024
@mrlubos
Copy link
Member

mrlubos commented May 16, 2024

Hey @omridevk, let me come back to this once I have the new clients working!

@omridevk
Copy link
Author

@mrlubos Yes I started to look at the branch you are working on, is there a plan to when this will be merged?

@omridevk
Copy link
Author

and thanks for the quick response :D

@mrlubos
Copy link
Member

mrlubos commented May 16, 2024

Oh now I feel shy, didn't know anyone else is watching that branch 😳 it's still missing a few things before I feel comfortable saying it's ready and can move onto other clients:

  • typed headers and body
  • body serialisers (it should automatically serialise form data for example, you pass the same JSON object to the request, request will know how to deal with it)

That's the bare minimum. I'm still thinking whether I want to provide any sort of migration layer or simply say generate a new client and replace the old one. And then some tests, clean up the code, and write documentation. 🚀

@omridevk
Copy link
Author

omridevk commented May 16, 2024

Well if the exposed API changes, ideally a codemod will be awesome.

Oh now I feel shy, didn't know anyone else is watching that branch 😳 it's still missing a few things before I feel comfortable saying it's ready and can move onto other clients:

Yeah I have a habit of looking at other people's source code 🧑‍💻

@omridevk
Copy link
Author

@mrlubos
Isn't there a way that as part of the config, a consumer can pass a fetch client that confirm to an interface? then you will not need to maintain a large list of clients, you could support the main one but allow the community to add more as time goes on.
For example, I do want to add a Ky client as well as Got is mainly used in Node, while Ky is used in browser env.

@mrlubos
Copy link
Member

mrlubos commented May 16, 2024

I thought about it. At that point, though, where would that client live? Would everyone write/provide their own clients? If so, it's still easier to provide a shared package @hey-api/client-{name}, no?

@mrlubos mrlubos changed the title Adding Got client support Got client May 16, 2024
@omridevk
Copy link
Author

that is non of your concern :) you can provide the most requested one, and the community can create their own packages with clients.
as long as it will easy to implement a client, I would be happy to publish a lib with got client for example and a ky-client to npm

@omridevk
Copy link
Author

It just not sure you would want to maintain all those clients, if you can offload some of the maintenance to other developers, it is always good :)

@mrlubos
Copy link
Member

mrlubos commented May 16, 2024

Yep, have to see if I can come up with a shared API. So far I got only one client (pun intended), it will become more clear after getting Axios working

@mrlubos
Copy link
Member

mrlubos commented May 16, 2024

It is my concern btw @omridevk because it impacts discoverability. Those clients should be listed somewhere, I don't want people to write their own clients if there are existing packages out there. The second issue that might come up is updating those clients as you point out. Maintaining them is an extra burden, but at least I could update them faster when needed than rely on 3rd party. If people start using a client and now it doesn't work with the latest openapi-ts version anymore, it prevents them from using the new features and they'll start looking for alternatives. There's a reason openapi-fetch exists, even though openapi-typescript-fetch came before that. I'd much rather host all major clients under single monorepo and people can maintain them, than rely on outside maintainers entirely. Got and Ky have each 10k+ stars and Got has 19m weekly downloads, that should be definitely part of the monorepo.

@omridevk
Copy link
Author

totally agree about ky and got, and I agree with your points, as always in programming, there are tradeoffs to any decisions :)

@omridevk
Copy link
Author

I do think that providing a way to bring your own client is a really good feature to have

@mrlubos
Copy link
Member

mrlubos commented May 17, 2024

Already working on a shared client interface, that's a good call @omridevk 🤝

@mrlubos
Copy link
Member

mrlubos commented May 21, 2024

@omridevk I just released the Fetch API client (demo). I imagine Got would look similar to this. Do you want it? 😄

@omridevk
Copy link
Author

@mrlubos awesome! I will, at the moment I can't really use this lib as the output doesn't include any file extension which fails when tsconfig is set to moduleResolution: "node16" as reported in my other ticket.
I was able to fix it in a fork, but I wasn't able to get the tests working as expected as I wanted to add some tests for this case.
Can you maybe assist in this matter?
Thanks, appreciate your help

@mrlubos
Copy link
Member

mrlubos commented May 26, 2024

Yeah let's do it! How did you implement it?

@omridevk
Copy link
Author

@mrlubos
Here is the PR:
#644

@omridevk
Copy link
Author

omridevk commented Jun 9, 2024

@mrlubos
I'll start implementing the Got client today, will share my progress as soon as I have some

@mrlubos
Copy link
Member

mrlubos commented Jul 21, 2024

@omridevk remind me, what did you end up doing with the Got client support?

@omridevk
Copy link
Author

I was able to pass got as the fetch implementation

@omridevk
Copy link
Author

Will share my code

@mrlubos
Copy link
Member

mrlubos commented Jul 24, 2024

@omridevk Do you think I should still publish a separate client for Got? I'm not sure if people will find it easy to discover this support otherwise. Definitely need to add at least a mention to docs too

@omridevk
Copy link
Author

omridevk commented Jul 25, 2024

@mrlubos
Here's the snippet I use to use Got/Ky as the fetch client.


import type {Got, Request as GotRequest, Response} from 'got'
import type {KyInstance} from 'ky'

export function createFetch(client: Got | KyInstance) {
  return async (request: Request) => {
    const url = isServer ? request.url.toString() : request.url.toString().replace('/ingress', '')
    // this is a thin wrapper to call either Ky or Got
    const result = await client[request.method?.toLowerCase() as 'get'](url, {
      headers: normalizeHeaders(request.headers) as unknown as Headers,
      ...(request.body && {json: await request.json()}),
    })
    return new Response(result.body, {
      headers: result.headers as unknown as Headers,
      status: isGotResponse(result) ? result.statusCode : result.status,
      statusText: isGotResponse(result) ? result.statusMessage : result.statusText,
    })
  }
}

function normalizeHeaders(headers: Request['headers']) {
  const out: GotRequest['headers'] | Headers = {}
  headers.forEach((value, key) => {
    out[key] = value
  })
  return out
}



This way I can use it with SSR and use Got in the server and Ky in the client.

@omridevk
Copy link
Author

omridevk commented Jul 25, 2024

I think it may be very useful to create a got/ky client, because I think the "fetch-client" has a lot of logic that is not required when using Got, as Got/Ky provide their own hooks and no need for interceptors and other implementation

@omridevk
Copy link
Author

omridevk commented Jul 25, 2024

My snippet probably has some issues and it doesn't cover all the use cases as I am not copying all the stuff that may be needed from the request, but it is easy to add missing properties, or find a better way of turning native request to Got request

@mrlubos
Copy link
Member

mrlubos commented Jul 25, 2024

Okay, will be a separate package at some point 🤝

@mrlubos mrlubos added the RSVP 👍👎 Explicit request for reactions to gauge interest in resolving this issue label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Client package related feature 🚀 New feature or request RSVP 👍👎 Explicit request for reactions to gauge interest in resolving this issue
Projects
None yet
Development

No branches or pull requests

2 participants