trpc / v10-playground

tRPC v10 procedure play
https://stackblitz.com/github/trpc/v10-playground?file=src%2Fserver%2Findex.ts,src%2Fclient.ts,src%2Fserver%2Frouters%2FpostRouter.ts&view=editor
13 stars 3 forks source link

[RFC] Simplify piping and get rid of `next()`-logic #8

Closed KATT closed 2 years ago

KATT commented 2 years ago

Would want some feedback from people that use middlewares a lot - is there any value of being able to infer a new context and be able to run next() at the same time?

I feel like all the next()-result style middlewares are generally application wide; things like error catching and such - am I wrong in that assessment?

cc @mmkal @anthonyshort

mmkal commented 2 years ago

I feel like all the next()-result style middlewares are generally application wide; things like error catching and such - am I wrong in that assessment?

Not all application wide in our case, we have some specific to certain sub-routers. Would that still be possible?

KATT commented 2 years ago

I feel like all the next()-result style middlewares are generally application wide; things like error catching and such - am I wrong in that assessment?

Not all application wide in our case, we have some specific to certain sub-routers. Would that still be possible?

Would those be fine as middlewares that didn't swap the context around?

mmkal commented 2 years ago

I feel like all the next()-result style middlewares are generally application wide; things like error catching and such - am I wrong in that assessment?

Not all application wide in our case, we have some specific to certain sub-routers. Would that still be possible?

Would those be fine as middlewares that didn't swap the context around?

I guess depends on your definition of fine - yes we could always find other ways to achieve the same thing, but right now trpc is not dictating what’s useful when. We have some cases where we’re swapping context to provide, for example, patient-specific or provider-specific helper props on the context. We also have middleware that reports provider api calls (not patient) to destinations like cloudwatch, kinesis etc.

I might be being overly protective of how awesome trpc is for us right now, but I kind of don’t want to lose anything of it!! Again I haven’t got my head around the proposed architecture but I’m wondering whether there’s a way for all of this to be just implementation, and keep the api the same. As a selfish user (albeit sometime contributor) I just would want any changes to be motivated by api shortcomings/new features more than an elegant/simple implementation. Or is the change for the sake of the blitzjs merge?

KATT commented 2 years ago

I guess depends on your definition of fine - yes we could always find other ways to achieve the same thing, but right now trpc is not dictating what’s useful when. We have some cases where we’re swapping context to provide, for example, patient-specific or provider-specific helper props on the context. We also have middleware that reports provider api calls (not patient) to destinations like cloudwatch, kinesis etc.

Thanks for detailing it, I'd like to have a solid migration path for ~most~ all usecases where the DX is better than before. I don't want to kill useases.

I might be being overly protective of how awesome trpc is for us right now, but I kind of don’t want to lose anything of it!! Again I haven’t got my head around the proposed architecture but I’m wondering whether there’s a way for all of this to be just implementation, and keep the api the same. As a selfish user (albeit sometime contributor) I just would want any changes to be motivated by api shortcomings/new features more than an elegant/simple implementation. Or is the change for the sake of the blitzjs merge?

mmkal commented 2 years ago

Thanks for detailing it, I'd like to have a solid migration path for ~most~ all usecases where the DX is better than before. I don't want to kill useases.

Great! Maybe it'd be helpful then in getting a bit more concrete with the use cases I'm thinking of:

What would this kind of thing look like in the new world:

export const appRouter = trpc
  .router<Context>()  
  .middleware(async ({next}) => {
    const start = Date.now()
    const result = await next()
    const durationMs = Date.now() - start
    logger.info({durationMs})
    return result
  })

or async_hooks magic:

import {AsyncLocalStorage} from 'async_hooks'

const fooStorage = new AsyncLocalStorage<Foo>()

const provideFoo = (foo: Foo, fn: () => T) => {
  return new Promise<T>((resolve, reject) => {
    fooStorage.run(foo, () => {
      fn().then(resolve, reject)
    })
  })
}

const useFoo = () => {
  const foo = fooStorage.getStore()
  if (!foo) throw new Error(`useFoo should be used within a callback passed to provideFoo`)
  return foo
}

export const appRouter = trpc
  .router<Context>()  
  .middleware(async ({next, ctx}) => {
    return provideFoo(ctx.getFoo(ctx.headers['x-foo']), () => next())
  })
  .middleware(async ({next, ctx}) => {
    return next({ctx: {bar: ctx.headers['x-bar']}})
  })
  .query({
    resolve: async ({ctx}) => {
      const foo = useFoo()   // .headers no longer on ctx, but can still summon via useFoo/async_hooks
      const bar = ctx.bar    // .bar is the only property on ctx now
      return {foo, bar}
    }
  })
KATT commented 2 years ago

Thanks for detailing it @mmkal, I will have a look during the week! I haven't used AsyncLocalStorage so it'll be fun!

KATT commented 2 years ago

Decided against this, for now, to simplify @mmkal's usecase.