Allow passing additional context to app()

[jacobwgillespie]

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]

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

const app = createTwirpServerless([...])
app.use(contextFromRequest)

[jacobwgillespie]

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]

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]

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]

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]

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]

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:

• Request / Response interface for TwirpServerless (probably renamed to TwirpCore).
• Context inference (basically typing use as a compose function) so middleware context is typesafe.

[tatethurston]

I think we ran into a transient error -- I need to look into it, but the merge into main passed, and this was the first failed check I've seen.

The clientcompat binary is vendored. I'm surprised my macos build (local) runs on ubuntu-latest (ci), but here we are 🤷

[jacobwgillespie]

this was the first failed check I've seen

Ah, this is because the clientcompat test doesn't run in CI, but only if the CODECOV_TOKEN environment variable is present:

TwirpScript/src/integration-tests/clientcompat/src/test.ts

Lines 13 to 16 in 413f01e

	// TODO: Run under GitHub CI
	if (process.env.CODECOV_TOKEN) {
	return;
	}

My PRs are coming from a forked repo, so GitHub isn't providing their builds access to secrets (a security feature), so the CODECOV_TOKEN isn't getting set, hence the failure. 🙂

[tatethurston]

🤦 I forgot about that hack. Sorry about that -- thank you for debugging this.