KevinEdry / nestjs-trpc

Providing native support for decorators and implement an opinionated approach that aligns with NestJS conventions.
https://NestJS-tRPC.io
MIT License
81 stars 8 forks source link

request: Add optional trpc-panel integration #7

Closed hrmcdonald closed 2 months ago

hrmcdonald commented 2 months ago

Wanted to ask if you'd be open to adding an optional dev tool integration. Much like @nestjs/graphql exposes an option for playground: true. I'd love to add an integration for trpc-panel.

Proposal

This approach works best since it always would be initialized after the latest iteration of the schema file is generated. The alternatives below might technically initialize with a previous iteration of the schema if called before the newer one is generated.

Alternatives

If you don't like this idea, I'd like to request that either:


Interested in your thoughts, would be happy to help put up a PoC PR for this if you like the idea.

KevinEdry commented 2 months ago

The @nestjs/graphql playground integration makes more sense to me because it is an official playground that will probably continue to be maintained and augmented with every change and update to graphql.

The trpc-panel on the other hand is a community project that has no guarantee to be updated in the feature, I am thinking about a scenario where trpc updates to a new version, and trpc-panel won't update for a couple of months to support the new version, this is problematic since if people are using this integration we will need to couple ourselves to another library release schedule to avoid breaking people's implementation.

I think a "Bring your own integration" makes more sense imo because of that, I can easily export the appRouter object if that will enable you to integrate with trpc-panel.

Wdyt? I've been working on this library for a while and to finally get some feedback on my implementation is amazing, so please don't shy away if you think my opinion is without merit.

hrmcdonald commented 2 months ago

Yeah, bringing your own integration definitely is the safer option from a maintenance perspective. It'd obviously also enable people to leverage whichever trpc-panel-like solution they prefer (trpc-openapi, trpc-devtools, etc...).

Just going the export route should definitely work fine enough. We might need to use an async import() to pull in the reference safely after a timeout in scenarios where the router file hasn't been generated yet (otherwise we'd hit a compilation error before it could), but I think that'd be OK.

A more NestJS way might be to ask the user to provide an injectable class in the module options that you'd call somewhat like how things work with context. You could just provide that class right away in the static forRoot for your own injection token. You could then inject that token later and call a method on the provided service instance only once with the combined appRouter after the router file is generated and trpc is running.

Maybe not with this name, but something like:

TRPCModule.forRoot({
  autoSchemaFile: "/src/@generated",
  context: AppContext,
  routerInitialized: TrpcPanelInitializer
}),

Similar to with contexts, you could expect any injectable class passed to something like this to implement an interface for something like:

export interface TRPCRouterInitializer {
  initialized(
    appRouter: CreateRouterInner<AnyRootConfig, ProcedureRouterRecord>,
  ): void;
}

That way we could just use DI to get the express app ref or whatever else we need from within our own class to stand things up more safely in response to the interface method getting called.

KevinEdry commented 2 months ago

It seems that all of them need the runtime appRoute so it probably won't be helpful to export the generate one, in terms of DX I was thinking something similar to how the HttpAdapterHost is managed:

  @Inject(HttpAdapterHost)
  private readonly httpAdapterHost!: HttpAdapterHost;

You just inject it with Dependency Injection, and check if it's there:

const httpAdapter = this.httpAdapterHost?.httpAdapter;
    if (!httpAdapter) {
      return;
    }

A better solution is to copy the DX of the graphql adapter for nestjs: https://docs.nestjs.com/graphql/quick-start#accessing-generated-schema

KevinEdry commented 2 months ago

@hrmcdonald It's been a productive Saturday! I've added a working solution in v1.3.5, here is the new documentation page for it: https://www.nestjs-trpc.io/docs/integrations

There is also a working example here: https://github.com/KevinEdry/nestjs-trpc/blob/main/examples/nestjs-express/src/trpc-panel.controller.ts

I don't know why but when I merge a PR it closes the issue automatically, can you confirm this works for you?

hrmcdonald commented 2 months ago

Ah I see, this approach seems much better than anything I've suggested. I didn't think to grab appRouter from HttpAdapterHost, but AppRouterHost is a nice straightforward way to do that. Will check this out tomorrow to confirm, but this looks great!

hrmcdonald commented 2 months ago

Just confirmed, works great!