Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Make createUser name argument default null #397

Merged
merged 2 commits into from
Jan 20, 2019

Conversation

nunovieira
Copy link
Contributor

This is the simplest fix for #396 I came up with.
But maybe name should be required, as in flow-yoga template?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jan 18, 2019

Hey @nunovieira thanks for this! It looks like our improved type generation in 0.5.0 is catching subtle type errors previously latent.

We ought to have a test suite for avoiding breaking the templates again in the future like that.

You're fix is certainly reasonable but there are alternatives to changing the model types, such as:

diff --git a/src/schema.graphql b/src/schema.graphql
index 31321e1..ca6e4d8 100644
--- a/src/schema.graphql
+++ b/src/schema.graphql
@@ -5,7 +5,7 @@ type Query {
 }

 type Mutation {
-  createUser(name: String): User!
+  createUser(name: String = null): User!
   createDraft(title: String!, content: String!, authorId: ID!): Post!
   deletePost(id: ID!): Post
   publish(id: ID!): Post
diff --git a/src/resolvers/Mutation.ts b/src/resolvers/Mutation.ts
index a66005f..b0bc1f9 100644
--- a/src/resolvers/Mutation.ts
+++ b/src/resolvers/Mutation.ts
@@ -1,7 +1,7 @@
 import { MutationResolvers } from '../generated/graphqlgen'

 export const Mutation: MutationResolvers.Type = {
-  createUser: (parent, { name }, ctx) => {
+  createUser: (parent, { name = null }, ctx) => {
     const id = ctx.data.idProvider()
     const newUser = { id, name, postIDs: [] }
     ctx.data.users.push(newUser)

I think using null default in the schema might be a nice way to solve whilst showing off that feature too. What do you think?

@jasonkuhrt jasonkuhrt self-requested a review January 18, 2019 01:34
@nunovieira
Copy link
Contributor Author

I didn't knew that feature!
We could do that. I leave that decision to you (the maintainers), but I think we should have the same schema in both templates.

Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

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

@nunovieira agree lets update both templates. Can you make those changes using the aforementioned GraphQL SDL null-default feature, thanks!

This is the simplest fix for prisma-labs#396 I came up with.
But maybe `name` should be required, as in `flow-yoga` template?
@nunovieira
Copy link
Contributor Author

I've amended the commit with your suggestion.
I didn't update the flow-yoga template because I got errors with name nullable:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/data.js:31:36

Cannot assign object literal to data because:
 • string [1] is incompatible with null [2] in property name of array element of property users.
 • string [3] is incompatible with null [2] in property name of array element of property users.

     src/data.js
 [1]  5│   { id: '1', name: 'Alice', postIDs: ['3', '4'] },
 [3]  6│   { id: '2', name: 'Bob', postIDs: [] },
       :
     28│   return `${idCount++}`
     29│ }
     30│
     31│ export const data: Data = { posts, users, idProvider }
     32│

     src/types.js
 [2]  9│   name: string | null;



Found 2 errors

I'm not that comfortable with flow to make that change.

@nunovieira nunovieira changed the title Make User.name optional Make createUser name argument default null Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants