Unable to set credentials property of fetch configuration

[andrewbeckman]

I would like to set the credentials property for the underlying fetch requests made by TwirpScript clients.

More generally, I think it might be a nicer design to expose the optional ability to set any of the fetch config. But I'm only blocked by the credentials right now as I'm implementing cookie based authentication.

[andrewbeckman]

@tatethurston just updating the issue to say that I realized I can workaround this for now by doing the following:

import { client } from "twirpscript"

client.rpcTransport = (url, opts) =>
  fetch(url, { ...opts, credentials: "include" })

Not sure what your thoughts are here, but this feels suboptimal to me insofar as I think it'd be great to extend ClientConfiguration.

[tatethurston]

Hey @andrewbeckman 👋 using client.rpcTransport like you found certainly works, but I agree that extending ClientConfiguration is more ergonomic.

Another workaround is using middleware:

client.use((context, next) => {
  return next({ ...context, credentials: "include" } as any);
});

But I think the intent is less clear here than with the rpcTransport override, and requiring users to make an unsafe cast isn't acceptable IMO.

Ideally I think we'd want something like:

client.credentials = "include"

or

client.options = { credentials: "include" }

[tatethurston]

The next piece is thinking about typing this -- we should be able to lean on TypeScript to prevent silly errors like:

// credential should be credentials
client.options = { credential: "include" }

I'm hesitant to force coupling to the full fetch interface, because that makes rpcTransport less flexible, but I could be convinced that flexibility isn't particularly valuable -- I'm not sure many (any?) clients will want to swap over to request response over websockets instead of fetch for instance. My original thinking was that some clients might want to provide axios or another fetch wrapper.

Making ClientConfiguration generic could enable fetch defaults with the ability to override:

ClientConfiguration<RpcOptions = RequestInit>

Though the mutable client export makes wiring this up on the client side less ergonomic and require a cast:

import { client } from 'twirpscript';

const myClient = client as ClientConfiguration<CustomOptions>

or

import { client, type ClientConfiguration } from 'twirpscript';

(client as ClientConfiguration<CustomOptions>).options = { ... };

That may be an acceptable for now for custom rpcTransport, and I can iterate on this further if someone opens an issue for a non fetch use case.

[andrewbeckman]

@tatethurston thanks for the detailed writeup! The motivation behind not being wed to fetch makes sense & I appreciate the complexity here. The idea of making ClientConfiguration generic seems neat. Given the popularity of cookie based auth, I could also see an argument that it deserves to be a 4th (optional) param under ClientConfiguration. In any case, I'm unblocked by the rpcTransport override, so this is by no means urgent for us. If you want to close this issue (or would like me to), it's fine with me!

[tatethurston]

Thanks @andrewbeckman. I'm going to research this a bit more. I may need to wait until bringing fetch into node.js moves a bit further. Right now, I don't want to force nodejs clients to pull in lib.dom.ts (#48). If TypeScript refactors their bundled types (https://github.com/microsoft/TypeScript/issues/43972) or fetch types land in @types/node this will be more straightforward to implement.