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

feat(enhancement): Upgrades generated graphql nexus file with more CRUD examples #174

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

mthomps4
Copy link
Contributor

@mthomps4 mthomps4 commented Jun 29, 2021

Description

Upgrades generated graphql nexus file with more CRUD examples and proper Input Types

Changes

GraphQL Nexus template was lacking, had bogus "SOMETHING" and User definitions.

Now generates:

  • Base Module

  • Query thing w/ user logged in guard

  • Query list('thing') w/ user logged in guard

  • Mutation createThing w/ isAdmin guard

  • Mutation updateThing w/ IsAdmin guard

  • ThingWhereInput

  • ThingWhereUniqueInput

  • ThingCreateInput

  • ThingUpdateInput

  • ThingOrderByInput

  • Generating a new app works

closes #173

@mthomps4 mthomps4 requested review from cball and kgajera June 29, 2021 18:45
@mthomps4 mthomps4 changed the title Upgrades generated graphql nexus file with more CRUD examples feat: Upgrades generated graphql nexus file with more CRUD examples Jun 29, 2021
@mthomps4 mthomps4 changed the title feat: Upgrades generated graphql nexus file with more CRUD examples feat(enhancement): Upgrades generated graphql nexus file with more CRUD examples Jun 29, 2021
@mthomps4 mthomps4 added the enhancement New feature or request label Jun 29, 2021
Copy link
Member

@cball cball left a comment

Choose a reason for hiding this comment

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

Looking great. Just a few minor questions

// export const UserRole = enumType({
// name: 'Role',
// members: Object.values(Role),
// });
Copy link
Member

Choose a reason for hiding this comment

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

it feels like we might delete this most of the time. I definitely see the value in outlining the Object.values example, but I did want to ask if you think this falls into the "you'll need this often" category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.
It is not an everyday thing, probably something better suited for Docs over a generated file.
Will opt to remove here.

Comment on lines 22 to 27
// Database Specific Fields
t.nonNull.id('id');
t.nonNull.date('createdAt');
t.nonNull.date('updatedAt');
// Model Specific Fields
t.nonNull.string('name');
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of trying to organize some of this. I worry that doing it this way may actually confuse things though. It requires you to ask yourself "is this a database field or a model field". Additionally, the name field that you have here doesn't have a custom resolver so it looks like it would be coming from the database.

Perhaps it would be better not to force the organization and just show an example of a custom resolver for a field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, those comments are a bit misleading.
name is in our generated example.

I'll remove the comments and leave it as is.

t.list.field('<%= plural.toLowerCase() %>', {
type: '<%= camelized %>',
t.field('<%= plural.toLowerCase() %>', {
type: list('<%= camelized %>'),
Copy link
Member

Choose a reason for hiding this comment

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

correct me if I'm wrong but I think both of these are equivalent. I feel t.list.field is slightly more straightforward due to the intellisense autocompletion and not requiring an additional import of the list function. thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We made this change when troubleshooting the other day.
It is the same...

I'll revert and save an import. 👍🏼

@@ -25,21 +33,32 @@ export const <%= camelized %>Queries = extendType({
type: 'Query',
Copy link
Member

Choose a reason for hiding this comment

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

while you are here, thoughts on moving this to queryField instead of extendType? Not required, but it does make it a bit more concise.

https://nexusjs.org/docs/api/query-field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to leave this one for another ticket.
We'll want to refactor to use query-field and mutation-field throughout in our user module as well to stay consistent.

I'll need to look at that syntax a bit, it looks like each piece would become its own exported queryType.

Copy link
Member

@cball cball left a comment

Choose a reason for hiding this comment

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

Awesome! :shipit:

@mthomps4 mthomps4 merged commit bbb581c into canary Jun 30, 2021
@mthomps4 mthomps4 deleted the mt-update-graphql-template branch June 30, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix generated GraphQL module template to create correct "where" inputs
3 participants