Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor + select support #28

Merged

Conversation

BitPhoenix
Copy link
Contributor

@BitPhoenix BitPhoenix commented Oct 28, 2022

Hi Omar I'm BitPhoenix, it's a pleasure to meet you. I wanted to start off by thanking you for creating such an awesome package! It's very useful and has helped me a lot. I really appreciate the hard work you continue to put into it.

My apologies that the refactor and fix are in one pull review, and that the commits are larger than normal. I'll make sure to separate them in the future.

Since this is quite a big pull request, please let me know if you'd like to hop on a voice chat to go through the changes together, in case that would help make things easier.

References

Description

I started out by trying to fix the missing select issue referenced here: omar-dulaimi/prisma-trpc-generator#13

To help me better understand the codebase, I started refactoring the prisma-generator.ts and transformer.ts files as I read through the code to get hands on and aid in understanding. Pretty small things like:

  • Extracting grouped logic into functions in prisma-generator.ts
  • Updating some variable / function names for clarity / consistency
  • Reorganizing function order in transformer.ts to decrease vertical distance
  • Extracting missing mongo type generation into a separate file + functions

After I finished performing the above changes and understood the codebase, I added functions to src/helpers/helpers.ts responsible for generating the types needed to support select. I plan on working on include next and will likely update the file / folder structure in a future pull request to represent that this group of functions is for adding the select related types (similar to how we now have a file dedicated to mongo missing type generation logic).

The screenshots below show the generated zod schema which will help a lot to explain what the end goal was of the code that was added to src/helpers/helpers.ts. The short version is:

Appropriate queries have a select property (Unique example is pictured)
Screen Shot 2022-10-28 at 3 32 10 PM

ModelSelect schema is generated. If the model has a many relation with another model, we include a union with the find many schema for that model. We also include the _count property
image

If the model has a single relation with another model, we union with args object schema for that model. If the model does not have a many relation with another model, _count will not be present.
Screen Shot 2022-10-28 at 3 37 48 PM

- Split generateObjectSchema logic into functions
- Rearranged method order to decrease vertical distance
Copy link
Owner

@omar-dulaimi omar-dulaimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @BitPhoenix
I'm glad you like this project and find it useful.

Let me start by saying that I don't know what is better, the description of the PR, the code refactor or the feature implementation!

Not only you did all that, but you also taught me a few things in the process; like vertical distance, the importance of high cohesion and separation of concerns at a practical level.

Thank you for that.

I left you just one small note(a typo) and a question in the review.

Also, if you could, maybe add a flag that defaults to false, that controls outputting select files. This way it would maintain the current behavior and the current amount of files people expect.

Other than that, I think you did great. I appreciate the time and effort you put into this.

src/prisma-generator.ts Outdated Show resolved Hide resolved
src/transformer.ts Show resolved Hide resolved
@Shahidul1004
Copy link
Contributor

Hi @BitPhoenix,
I am getting error for circular relations in select.
I integrated this PR to my tester app. I am getting full type support in vscode but APIs are not working. See the schemas. If I break the circular relation, then the APIs will work.
Screenshot from 2022-11-02 00-38-33
Screenshot from 2022-11-02 00-44-25

@BitPhoenix
Copy link
Contributor Author

BitPhoenix commented Nov 2, 2022

Hi @BitPhoenix, I am getting error for circular relations in select. I integrated this PR to my tester app. I am getting full type support in vscode but APIs are not working. See the schemas. If I break the circular relation, then the APIs will work. Screenshot from 2022-11-02 00-38-33 Screenshot from 2022-11-02 00-44-25

Hey Shahidul1004 nice to meet ya! Thank you for testing this out and submitting the very helpful information.

Interestingly when I try it out on my end I don't have any issues, I suspect it working on my end is a side effect of node's behavior for supporting circular imports:

Will attach screenshots of it working on my end below as a sanity check

image

image

Do you happen to have your code pushed to a branch somewhere where the three of us can test it out / reproduce the error to make it easier to work on a fix? I don't get the error in my current environment but can definitely see this being an issue in other ones. So just want to make sure I can be in one where the error occurs for fixing / testing any fixes 🙂

I'm curious if there's any difference if you manually update the generated schema to this:

export const PostFindManySchema = z.object({
  where: PostWhereInputObjectSchema.optional(),
  orderBy: PostOrderByWithRelationInputObjectSchema.optional(),
  cursor: PostWhereUniqueInputObjectSchema.optional(),
  take: z.number().optional(),
  skip: z.number().optional(),
  distinct: z.array(PostScalarFieldEnumSchema).optional(),
})
.strict();
Object.assign(PostFindManySchema,  PostFindManySchema.extend({ select: PostSelectObjectSchema.optional() }));

Curious if you'll get the same error with this snippet. If no error this might be a potential fix, the only downside being that the select is not available in the type if you hover over the imported PostFindManySchema. Note that select is actually present and working on the exported schema - can be verified by parsing an object containing select first, then commenting out Object.assign and testing again. It's just not shown on the type when hovering with mouse, but is actually part of the exported / imported zod schema.

If this doesn't make a difference and we get the same error, we'll have to brainstorm a bit more on how to resolve this.

- Renamed handleMongoDbRawOperationsAndQueries -> addMissingInputObjectTypesForMongoDbRawOpsAndQueries
- Moved mongodb input type object generation call into addMissingInputObjectTypes
- Extracted aggregate type input object type generation into aggregate-helpers
- Function parameter typo fix
@BitPhoenix
Copy link
Contributor Author

BitPhoenix commented Nov 2, 2022

@omar-dulaimi Update on most recents commits:

  • Merged in @Revitate aggregate support
  • Extracted aggregate support to helpers/aggregate-helpers.ts
  • Renamed helpers.ts to select-helpers.ts
  • Now calling mongodb input object type generation / pushing from addMissingInputObjectTypes
  • Fixed function parameter typo

Before merging: note that discussion / investigation with @Shahidul1004 is pending to resolve the issue they are experiencing

@BitPhoenix
Copy link
Contributor Author

BitPhoenix commented Nov 2, 2022

Hey @BitPhoenix I'm glad you like this project and find it useful.

Let me start by saying that I don't know what is better, the description of the PR, the code refactor or the feature implementation!

Not only you did all that, but you also taught me a few things in the process; like vertical distance, the importance of high cohesion and separation of concerns at a practical level.

Thank you for that.

I left you just one small note(a typo) and a question in the review.

Also, if you could, maybe add a flag that defaults to false, that controls outputting select files. This way it would maintain the current behavior and the current amount of files people expect.

Other than that, I think you did great. I appreciate the time and effort you put into this.

Thank you so much for the kind words 🙏 And for the quick review!

I'm happy to hear that I was able to share a few things, I learned a lot from you about how prisma generators work! Very grateful that I got to give back in some way.

"Also, if you could, maybe add a flag that defaults to false, that controls outputting select files. This way it would maintain the current behavior and the current amount of files people expect."

Nice idea - I'll work on this one next!

@Shahidul1004
Copy link
Contributor

Hi @BitPhoenix
Hope you are doing great.
I got a solution. We need to call PostSelectObjectSchema as a lazy function inside of PostFindManySchema. Because we are calling PostSelectObjectSchema inside PostFindManySchema, but PostSelectObjectSchema is defined later.

This circular relation exists in the Post model but not in the User model. So we can call {Model}SelectObjectSchema as a lazy function inside of {Model}FindManySchema if the {Model} has many-to-one relation with other models. But it would be fine, if we always lazily call {Model}SelectObjectSchema inside {Model}FindManySchema.
Screenshot from 2022-11-02 16-49-39

@Shahidul1004
Copy link
Contributor

Hey @BitPhoenix
I was trying to add include support for this repo and, it seems like I have succeeded. I was wondering if you are also working on the same thing. If that's not the case then I might create a PR for include support.

…s when the model in question has a many to one relation with other models.

Co-authored-by: Shahidul1004 <Shahidul1004>@users.noreply.github.com
@BitPhoenix
Copy link
Contributor Author

Hi @BitPhoenix Hope you are doing great. I got a solution. We need to call PostSelectObjectSchema as a lazy function inside of PostFindManySchema. Because we are calling PostSelectObjectSchema inside PostFindManySchema, but PostSelectObjectSchema is defined later.

This circular relation exists in the Post model but not in the User model. So we can call {Model}SelectObjectSchema as a lazy function inside of {Model}FindManySchema if the {Model} has many-to-one relation with other models. But it would be fine, if we always lazily call {Model}SelectObjectSchema inside {Model}FindManySchema. Screenshot from 2022-11-02 16-49-39

Fantastic solution and explanation! Added a commit with your solution: 12b4b09

Added co-authored by line for you in the commit message but not sure if it worked properly

@BitPhoenix
Copy link
Contributor Author

Hey @BitPhoenix I was trying to add include support for this repo and, it seems like I have succeeded. I was wondering if you are also working on the same thing. If that's not the case then I might create a PR for include support.

That's fantastic! I was going to work on include later this week so nope all good! I'll add the feature flag request today that omar requested for select generation and hopefully we can get PR one merged in and then we can look over your include PR next 🎊

You probably already did this but just in case, one suggestion I have is to put include function code under helpers/include-helpers.ts and then call the function from helpers.ts addMissingInputObjectTypes, basically just keeping the same convention this pr introduces for organized missing input object type generation logic: so far we have aggregate, select, mongodb and with yours we'd also have a dedicated file for include 🙂

@BitPhoenix
Copy link
Contributor Author

@omar-dulaimi Small update: added a commit with @Shahidul1004's fix for the circular relation. Last part is to add the flag support for select generation which I will add today so that we can tackle @Shahidul1004's include functionality PR next

…lect schemas will be generated and whether the select property is added to the relevant query schemas
@BitPhoenix
Copy link
Contributor Author

BitPhoenix commented Nov 2, 2022

@omar-dulaimi Most recent commit contains isGenerateSelect generator config option that controls whether Select schemas are generated, and whether the select property is available on query schemas (defaults to false)

@Shahidul1004 Might be worth reviewing that commit as I imagine Omar might potentially want include to be flag controlled as well

@BitPhoenix
Copy link
Contributor Author

Ready for re-review ✅

Copy link
Owner

@omar-dulaimi omar-dulaimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you guys for your hard work on this PR so far. I didn't find anything major to comment on in the review, since everything was taken care of. Great catch though from @Shahidul1004 for the cyclical ref issue, it's kind of an annoying issue I faced in another generator. Glad that you guys managed to solve it with Zod.

README.md Outdated Show resolved Hide resolved
src/transformer.ts Outdated Show resolved Hide resolved
@BitPhoenix
Copy link
Contributor Author

Ready for re-review: ✅

  • Updated README isGenerate -> isGenerateSelect
  • Updated default value to be '' instead of undefined

@omar-dulaimi omar-dulaimi merged commit 94e5c5c into omar-dulaimi:master Nov 4, 2022
@omar-dulaimi
Copy link
Owner

Released in https://github.com/omar-dulaimi/prisma-zod-generator/releases/tag/0.6.0

Thanks for your efforts

@BitPhoenix
Copy link
Contributor Author

@omar-dulaimi Thank you for all of your time and insightful comments on this pull request! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants