tatethurston / TwirpScript

A protobuf RPC framework for JavaScript and TypeScript
MIT License
141 stars 14 forks source link

Allow passing additional context to `app()` #190

Open jacobwgillespie opened 2 years ago

jacobwgillespie commented 2 years ago

This PR allows passing additional keys to context as the last argument to the app() returned from createTwirpServer or createTwirpServerless. This allows you to pass external context into the RPC call at the point you invoke the app. I've implemented it for both TwirpServer and TwirpServerless, but I think it's most useful for TwirpServerless, where you are building a custom RPC server on top of TwirpScript:

const app = createTwirpServerless([...])

export function handler(request: CustomRequestType) {
  const customContext = contextFromRequest(request)
  return app({...}, customContext)
}

The types are such that the new generic parameter for app<Context>() should only appear if the extraContext is provided. And the implementation is such that the extra context is spread into the context before TwirpScript's default context fields and before middleware run, so those existing fields should always have the values they have currently.

tatethurston commented 2 years ago

How does this differ from having contextFromRequest as the first registered middleware?

const app = createTwirpServerless([...])
app.use(contextFromRequest)
jacobwgillespie commented 2 years ago

Nice, yeah the difference is that you need some way to get context from the point at which you call the app into the RPC call, basically the request might not be the only thing from the surrounding closure that you need to pass down.

Using this in a Cloudflare Worker, the serverless handler looks something like this:

interface Env {
  exampleBinding: DurableObjectNamespace
}

const rpc = createTwirpServerless([...])

const handler: ExportedHandler<Env> = {
  async fetch(request, env) {
    const url = new URL(request.url)
    const res = await rpc({
      method: request.method,
      body: new Uint8Array(await request.arrayBuffer()),
      headers: Object.fromEntries(request.headers),
      url: url.pathname,
    })
    return new Response(res.body, {
      headers: new Headers(res.headers as any),
      status: res.statusCode,
    })
  },
}

That's translating the web fetch API / Cloudflare Worker request and response types into something that TwirpServerless understands.

But there's an additional env argument that contains important data (bindings to durable objects, R2 buckets, application secrets, etc) and I need some way to pass that down to the service handlers.

So after this PR, you could do something like this:

const handler: ExportedHandler<Env> = {
  async fetch(request, env) {
    const url = new URL(request.url)
    const res = await rpc({
      method: request.method,
      body: new Uint8Array(await request.arrayBuffer()),
      headers: Object.fromEntries(request.headers),
      url: url.pathname,
-   })
+   }, {env})
    return new Response(res.body, {
      headers: new Headers(res.headers as any),
      status: res.statusCode,
    })
  },
}

And now env is available in all handlers.

The other immediate use-case is I might need to pass the "real" request down to a handler for processing, so I can access the original request class. That might be something like await rpc({...}, {rawRequest: request}), etc.

jacobwgillespie commented 2 years ago

I think I might have fixed CI by installing clientcompat, though for some reason GitHub Actions isn't firing an event for the new commit. 🙂

tatethurston commented 2 years ago

Awesome, thank you for the example.

I've been thinking about reworking TwirpServerless so that it conforms to a fetch-like interface. Then we could drop all the boilerplate you currently need and your example would become:

interface Env {
  exampleBinding: DurableObjectNamespace
}

const rpc = createTwirpServerless([...])

const handler: ExportedHandler<Env> = {
  fetch(request, env) {
    return rpc(request);
  }
}

Then I think we could leverage middleware by splatting request with env:

interface Env {
  exampleBinding: DurableObjectNamespace
}

const rpc = createTwirpServerless<Env>([...])
+ rpc.use((req, ctx, next) => {
+   ctx.env = req.env;
+   return next();
+ });

const handler: ExportedHandler<Env> = {
  fetch(request, env) {
-   return rpc(request);
+   return rpc({ ...request, env });
  }
}

What do you think about this?

jacobwgillespie commented 2 years ago

Then I think we could leverage middleware by splatting request with env

I think that could work, and actually I think that would work today, without needing to conform to the fetch spec:

app.use(async (request, context, next) => {
  const {env} = request as unknown as {env: Env}
  context.env = env
  return next()
})

async fetch(request, env) {
  const res = await app({
    method: request.method,
    body: new Uint8Array(await request.arrayBuffer()),
    headers: Object.fromEntries(request.headers),
    url: url.pathname,
    env,
  } as InboundRequest)
  ...
}

The difficulty with that approach is the type-casts - without them today, you get errors like Object literal may only specify known properties, and 'env' does not exist in type 'InboundRequest'., and I believe you'd get the same error switching from InboundRequest to fetch's Request interface.

By putting the extra context on another function parameter, you can let TypeScript infer the type, or you can specify the type you expect in the generic as a safety check:

// now TypeScript will check that extraContext is of type ExtraContext
await app<ExtraContext>(request, extraContext)
tatethurston commented 2 years ago

The difficulty with that approach is the type-casts - without them today, you get errors like Object literal may only specify known properties, and 'env' does not exist in type 'InboundRequest'., and I believe you'd get the same error switching from InboundRequest to fetch's Request interface.

Yeah this would work today, you would need (roughly) the following types to make it work:

// something like this (hopefully there is a first classed type)
import type { Request } from 'cloudflare';
// or this if not
type Request = Parameters<ExportedHandler['fetch']>[0];

interface Env {
  exampleBinding: DurableObjectNamespace
}

interface Context {
  env: Env;
}

const services = [...];
const rpc = createTwirpServerless<Request & Context, typeof services, Context>(
  services
);
rpc.use((req, ctx, next) => {
 ctx.env = req.env;
 return next();
});

const handler: ExportedHandler<Env> = {
  fetch(request, env) {
    return rpc({ ...request, env });
  }
}

The types aren't as ergonomic as I would like though. The main problem is Typescript generic inference is all or nothing -- there is a long standing TS issue to partially apply generics and infer others.

tatethurston commented 2 years ago

I'm open to this change. I'd like to look into a few refactors I have in mind to see how it fits into that broader context. Specifically: