KevinEdry / nestjs-trpc

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

request: Add `imports` option to router decorators and/or root module #8

Open hrmcdonald opened 1 month ago

hrmcdonald commented 1 month ago

One roadblock we've hit investigating using this library, which we'd really love to do, is scenarios where the input/output zod schemas are not completely defined within their router file.

One option would be to somehow be able to analyze and infer relevant import statements...which I could see being pretty difficult. So if that is not a viable option, perhaps some simple options for an imports array could be exposed on the module and @Router decorator where we could provide import strings we'd like injected into the generated schema file so more complex schemas defined elsewhere could be used easily.

Examples

Here are two examples of where some kind of solution here is a must for us:

1) We use drizzle and generate various zod schemas from our tables using drizzle-zod. Even if we did this within a router file (which wouldn't be nice already), we'd still need to be able to import our drizzle table definition and drizzle-zod functions in order for that complex schema to work in the generated router file.

2) We have some factory functions that let us define some psuedo-generic structures for certain types of queries. They add a common structure to common import props for queries.

So for example the query input might be:

@Query({ output: queryAllPaginatedSchema(dogSchema) })
  async findAll(): string {

Where paginatedQuerySchema returns a schema with pagination options, sorting options, and some minor common filtering options for the provided schema object param. For this to work, we'd need querySchema() also imported into the generated file.

Proposal

I'm not sure of the complexity involved in analyzing files to infer the proper imports to include. So I'll just demo how the simpler solution I proposed could work - allowing us to declare import statement strings.

This could be an option both on the root module options (for global imports so they don't need to be made on every router):

TRPCModule.forRoot({
  autoSchemaFile: './src/@generated',
  imports: [
    `import { queryOneSchema, queryAllSchema, queryAllPaginatedSchema } from '../app/core/zod.ts';`
  ]
}),

And in the @Router decorator for imports specific to one router file perhaps something like:

@Router({
  imports: [`import { dogSchema } from '../app/dogs/dtos.ts';`]
})
export class DogsRouter {

These import strings could then just be injected into the top of the generated router file so the typing all works out. Today as is the input/outputs get included but things don't really work from there because the proper imports are not present.


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

KevinEdry commented 1 month ago

Thanks for the detailed issue! I'm so glad to see people interacting with the library! For your suggestion, the current implementation does take into account if you have schemas imported from different files, you can store a schema (no matter how convoluted) in other files and it should know to pick them up and flatten it at the end so you don't need to define the imports, look at the example router here: https://github.com/KevinEdry/nestjs-trpc/blob/main/examples/nestjs-express/src/user.router.ts

The output is in a different file altogether, and it does pick it up when generating the schema.

The queryAllPaginatedSchema function use-case is something I didn't account for, I can change the functionality to import exactly what you've put in the inputs/outputs instead of trying to flatten the schema and then figure out the relative path to them.

Is the library currently useable without the function zod schema use-case?

hrmcdonald commented 1 month ago

Ok that makes a ton of sense. I should've looked into the mechanism you were using there a bit closer. I just tested out one of those functions and noticed and was running into that issue before posting this.

Is the library currently useable without the function zod schema use-case?

We just started looking into this a bit closer today in a green-field project. Everything else other than these two issues I opened seems to be working great so far. We'll be sure to report on anything else we might run into as we use it some more.

KevinEdry commented 1 month ago

I've fixed some of the nested schemas functionality in the PRs i've linked, still have to figure out the generic functions use-case. To be fully transparent, here are some of my open questions that I am still struggling to answer:

  1. I want to isolate the generated server.ts, thats why I went with the schema flattening approach, how can I do that with function that might invoke other functions within them?
  2. If I can't flatten functions, then importing them to the server.ts file is the next obvious solution, but what if you didn't export the function you are using? for example if you created the function within the @Router file.
  3. What should happens if the function you are using is declared as part of a service or a class?

I'm still thinking those questions through and am open for suggestions 😁.

hrmcdonald commented 1 month ago
  1. I want to isolate the generated server.ts, thats why I went with the schema flattening approach, how can I do that with function that might invoke other functions within them?

I don't know enough about how you are achieving this today. I guess you could maybe attempt to infer the type of the result of a function (which would include the generic info since the input would also be provided there) and work backwards from that? that might be difficult or run into a lot of edge cases though potentially?

  1. If I can't flatten functions, then importing them to the server.ts file is the next obvious solution, but what if you didn't export the function you are using? for example if you created the function within the @Router file.

Expecting the function to be explicitly exported makes perfect sense here IMO. I'm not sure how you'd do it consistently any other way. The rule could basically be that functions used within inputs/outputs should not be declared inline within the decorator. Instead, they must be exported from somewhere so that they could in turn be easily imported into the server.ts file correctly.

  1. What should happens if the function you are using is declared as part of a service or a class?

This feels like another thing that is solved by just drawing the line at functions used for inputs/outputs must be standalone exported functions.

I don't think there'd be a clear sane way to interpret allowing class methods. Because what instance of those classes would even be exposed for use within the server.ts file to be evaluated by trpc? You'd have to know the deps to properly initialize and everything. Doesn't seem sane or viable to allow that with direct imports. That'd only maybe work through type inference of some kind if that route worked out?

KevinEdry commented 1 month ago

Before getting into things, let me just say thanks for your detailed reply, those kind of discussions are a tremendous help with shaping this library!

The way I am generating the zod schema is using ts-morph to look at your code, and then flattening the zod schema by copying your implementation throughout your imports (see types.utils.ts for the actual implementation).

Inferring types is the first thing I tried to do when developing the library, but I quickly figured out that the generated file needs to contain the actual zod schema implementation, and going backwards from the inferred type is too complex especially for zod.

The whole idea of this library is to let engineers write trpc routers with nestjs idioms as they would without any special rules other than the opinionated way of nestjs. Drawing a line and saying "you have to export your inputs in a certain way for this to work" defeats the purpose.

I can see doing the same thing we are doing with zod schemas for functions, basically inlining the function implementation as an arrow function and flattening it throughout your imports.

I think that will be a good first step, and after that we can think about solving the issue of class methods which is (and I hope it is) a rare edge-case.

I'll have one more issue to finish up, and i'll start digging into this one to hopefully have a working implementation by the end of the week.

hrmcdonald commented 1 month ago

Ah ok, based on that description of the way you made that work for raw schema objects, I could see how applying the same concepts could work with functions as well. As long as it's able to transform nested function calls into something like "inlined" arrow functions as well, then I think that could work out pretty well.

The only place it might break down is where there are conditional calls that could maybe end up in a loop if you attempted to flatten them. In our query schemas for example we have the capability to and/or conditions like string or number field comparisons with other where-like clauses. I'm not sure if it'd be simple to handle that when attempting to flatten functions.

I understand that is probably a pretty complex dynamic use-case of zod input schemas here, but hopefully this stress test the usage here some more. We're essentially bringing this over from some older graphql solutions, so we're still finalizing what that looks like in zod/trpc, but have the concepts defined on paper. I'll see if we can share an example that showcases this scenario sometime later this week if that'd be helpful for your testing/experimentation.

And thank you again for looking into these issues and edge-cases, thinking through them over feedback here, and working to implement the best path forward!

hrmcdonald commented 1 month ago

OK, update on the looping. It seems like while zod handles evaluating loops fine, other trpc integrations don't work well with that kind of schema well (trpc-panel for example). So we might move away from that and avoid exposing any possible loops anyways.

KevinEdry commented 1 month ago

I've been trying to solve this for a while without success (been stuck on type imports). I'll implement a feature that lets you add imports as string from the module config the way you suggested. I could always support it and once I solve the functions issue you won't need to declare them there anymore.

This should be available this weekend.

KevinEdry commented 1 month ago

Should be fixed in v1.5.0, it should import the missing functions automatically, but if it doesn't, you can use the new schemaFileImports to append those imports.

hrmcdonald commented 1 month ago

Thanks @KevinEdry, just wanted to report that the function imports now generate correctly when the file they are imported directly from their source file! It does not work when imported from any kind of barrel file. That seems like a different issue that I believe already has an open issue, but I just wanted to let you know for awareness in case that helps at all.

KevinEdry commented 1 month ago

Oh, let me reopen this issue, i'll fix the barrel file thing. Thanks for letting me know!