More typesafety for GraphQL code #6167

mattkrick · 2022-03-04T02:50:16Z

Run the following on the private schema & see the response times for various services

query {
  ping {
    rethinkdb
    postgres
  	redis
  }
}

Today, we don't get a lot of type safety when we create objects with graphql

We must remember to add types to the source
args are broken until we upgrade to v16
We have to remember to include the context
The return value of a resolver (e.g. {taskId}) isn't typed at all

So, i took a shot at what all the cool kids do these days.

The schema is created using an SDL. You write GraphQL & it just creates the objects for you. All that's left are the resolvers
codegen turns that SDL into types that you can apply to your resolvers
the source value of a resolver (the first param, e.g. {taskId}) is called a "Model", which can be thought of like a database object vs. an object returned from graphql. that separation is nice. I'm calling them Sources in this PR so the name matches what GraphQL calls them.

Reading material:

Would love to get some feedback!

mattkrick · 2022-03-04T02:51:22Z

codegen.json

+      "config": {
+        "mapperTypeSuffix": "Model",
+        "mappers": {
+          "PingPayload": "./ping#PingPayload"


each model must be defined here, which is a bit annoying, but makes sense since it needs to be included in the built types

This part is a bit annoying, but currently we mostly use any for it.

I would LOVE to remove this sharp edge.
the problem I face is that sometimes a Source comes from a database entity. Sometimes, it's just the return value from a mutation or query. I still haven't come up with a good enough rule to automate it. Once we do, we can write a script that automates this.

mattkrick · 2022-03-04T02:56:53Z

packages/server/graphql/intranetSchema/intranetSchema.ts

+
+const schema = mergeSchemas({schemas: [legacySchema], typeDefs, resolvers: resolverMap})
+
+const updateSchema = async () => {


there's probably a better way to create the schema SDL...

mattkrick · 2022-03-04T02:57:22Z

packages/server/graphql/intranetSchema/intranetSchema.ts

+const legacySchema = new GraphQLSchema({query, mutation, types: rootTypes})
+
+const typeDefs = loadFilesSync(
+  path.join(__PROJECT_ROOT__, 'packages/server/graphql/intranetSchema/typeDefs/*.graphql')


i really like this part. no more including files 1 by 1. just include the whole directory

mattkrick · 2022-03-04T02:58:47Z

packages/server/graphql/intranetSchema/resolvers/PingPayload.ts

+    return res ? duration : -1
+}
+
+const resolverMap: PingSuccessResolvers = {


these aren't the best examples because they're so simple, but if you try to return something that doesn't resolve to a number it'll throw an error, which is great!

mattkrick · 2022-03-04T03:00:24Z

packages/server/graphql/intranetSchema/resolvers/ping.ts

@@ -0,0 +1,7 @@
+import {QueryResolvers} from '../resolvers/types'
+
+export interface PingPayload {}


not the greatest example, but say we wanted to just return {taskId}, we could define that here

mattkrick · 2022-03-04T03:01:38Z

packages/server/graphql/intranetSchema/typeDefs/ping.graphql

@@ -0,0 +1,10 @@
+union PingPayload = ErrorPayload | PingSuccess


is writing a file like this easier than the old way?

scripts/dev.js

mattkrick · 2022-03-15T15:43:43Z

packages/server/graphql/intranetSchema/sdl/resolvers.ts

+  return collector
+}
+
+const resolverMap: Resolvers = {


@blimmer this is my attempt to emulate what you were doing with that codegen script

my current thinking is to have 3 folders inside /graphql:

public

public/queries

public/mutations

public/subscriptions

public/types

private

private/queries

private/mutations

private/types

github

queries

mutations

gitlab

queries

mutations

the above won't touch the current hierarchy so we can gradually move over to this new format.

blimmer

I'm submitting these initial ideas for now, going to shift back over to #6183 to complete that work, then come back over here to make additional progress.

codegen.json

scripts/dev.js

blimmer · 2022-03-15T17:15:44Z

codegen.json

@@ -12,6 +12,17 @@
      "schema": "packages/server/graphql/nestedSchema/GitLab/gitlabSchema.graphql",
      "documents": "packages/server/graphql/nestedSchema/GitLab/queries/*.graphql",
      "plugins": ["typescript", "typescript-operations", "add"]
+    },
+    "packages/server/graphql/intranetSchema/sdl/resolverTypes.ts": {


question: how do we get rid of this SDL folder?

Dschoordsch · 2022-03-18T17:14:30Z

packages/server/graphql/intranetSchema/intranetSchema.graphql

@@ -0,0 +1,5186 @@
+type Query {


+1 are we planning to split the schema up? Having everything in one giant file makes editing and reviewing harder.

Yep! Every new piece of business logic will have its own .graphql typeDef. We'll use a tool that merges all those typeDefs together into a generated schema that we won't have to look at or touch.

I finished the refactor & left all the legacy stuff in a big _legacy.graphql for the sake of time.
https://github.com/ParabolInc/parabol/pull/6228/files#diff-77adac95bcf72265d1a92f5b3932ccf2b8a6055e41e1baf2337fd7c5ee8fc830

Dschoordsch · 2022-03-18T17:17:53Z

packages/server/graphql/intranetSchema/sdl/mutations/updateEmail.ts

@@ -0,0 +1,48 @@
+import getRethink from '../../../../database/rethinkDriver'


+1 I find the folder structure a bit confusing. Why do we put 'sdl' in there? Schema definition language is what will end up in 'typeDefs' folder

Agreed!
What do you think of this hierarchy?

inside /graphql:

public

public/queries

public/mutations

public/subscriptions

public/types

private

private/queries

private/mutations

private/types

github

queries

mutations

gitlab

queries

mutations

See https://github.com/ParabolInc/parabol/pull/6228/files#diff-0281cf5ab4a0b4d7573df4f26731ffcd6bf36bdbc723eb82d1f0d83dd667e4d5 for a test implementation

Dschoordsch · 2022-03-18T17:31:59Z

packages/server/types/webpackEnv.ts

@@ -0,0 +1,386 @@
+// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+// @ts-nocheck
+// Type definitions for webpack (module API) 1.16


+1 Wouldn't you put type definitions in d.ts files? How does this differ from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/webpack-env/index.d.ts ?

i couldn't figure out how to import this without using the /// references pattern, which is deprecated. It also had some incorrect types, so i figure I'd just copy it over & remove the dep

Dschoordsch · 2022-03-18T17:34:10Z

scripts/dev.js

@@ -45,7 +45,10 @@ const dev = async (maybeInit) => {
    await Promise.all([removeArtifacts()])
  }
  // Enable this if you're creating new github schemas


-1 Comment is not up to date anymore

Dschoordsch · 2022-03-18T18:03:06Z

scripts/dev.js

-  // await generate(codegenSchema)
+  try {
+    await generate(codegenSchema)
+  } catch {}


-1 Do we really just want to swallow all errors? If so, there should be a comment why

Dschoordsch · 2022-03-18T19:49:46Z

codegen.json

+      "config": {
+        "mapperTypeSuffix": "Model",
+        "mappers": {
+          "PingPayload": "./ping#PingPayload"


This part is a bit annoying, but currently we mostly use any for it.

Dschoordsch

Looks good overall. The source mapping in codegen.json is a bit annoying and having definitions spread across more files, but overall it's probably the way to go.

I wish there was something like typebox for GraphQL instead, but the GraphQL types don't contain enough information and with codegen we're on a more mainstream path.

Dschoordsch · 2022-03-18T19:57:59Z

packages/server/graphql/intranetSchema/sdl/queries/logins.ts

+
+const logins: QueryResolvers['logins'] = async (_source, args) => args
+
+export default logins


-1 You should delete graphql/intranetSchema/queries/logins.ts

i saved most deletions for the subsequent PR. https://github.com/ParabolInc/parabol/pull/6228/files#diff-0281cf5ab4a0b4d7573df4f26731ffcd6bf36bdbc723eb82d1f0d83dd667e4d5

Dschoordsch · 2022-03-18T20:11:51Z

codegen.json

+      "plugins": ["typescript", "typescript-resolvers", "add"],
+      "config": {
+        "contextType": "../../graphql#InternalContext",
+        "mappers": {


+1 We might want to set "defaultMapper": void or similar to avoid the case where people don't define it.
We could even define export type No_model_added_in_codegen_json = {} and set it as default mapper, just so developers are pointed to it more directly.

i like the idea of forcing an explicit mapper!
i'll play around with the subsequent PR to test void and never and see which one works best.

Dschoordsch · 2022-03-18T20:12:46Z

packages/server/graphql/intranetSchema/sdl/types/PingableServices.ts

+  return res ? duration : -1
+}
+
+export type PingableServicesSource = Record<string, never>


-1 I think if there is no input, it should be undefined or void instead.

if we return undefined then the graphql traversal won't continue, it only continues on objects.

To test: remove return {} from ping.ts.

mattkrick · 2022-03-21T17:16:30Z

looks like master fails the build now, but not because of this.
gonna merge to master anyways & we can triage that later...

all issues that weren't fixed here (folder hierarchy, explicit Source types) will be fixed or trialed in the following PR: #6228

feat(ping): try new typed graphql pattern

2fde161

mattkrick marked this pull request as draft March 4, 2022 02:50

mattkrick added 2 commits March 3, 2022 18:55

feat(ping) add internal context

48dcc95

fix(ping) add missing paren

9d03215

mattkrick commented Mar 4, 2022

View reviewed changes

mattkrick added 7 commits March 4, 2022 11:54

feat(sdl): require.context whole directories

8f6d827

feat(sdl): move updateEmail to new pattern as example

e477235

feat(sdl): move logins to sdl to explore mappers

6c7eebf

feat(sdl): rename source

aa84a62

feat(sdl): move typeDefs to sdl

07c339d

feat(sdl) move update schema logic

88dc187

feat(sdl) fix include patterns

880c83f

mattkrick commented Mar 15, 2022

View reviewed changes

blimmer reviewed Mar 15, 2022

View reviewed changes

mattkrick marked this pull request as ready for review March 17, 2022 17:56

mattkrick added 4 commits March 17, 2022 11:10

add resolverTypes to gitignore

3f5dbab

ignore resolverTypes

1871697

Merge branch 'master' into feat/4754/pingpongwallawallabingbong

8871bde

feat(ping): make ping more robust

a16684b

mattkrick requested a review from Dschoordsch March 17, 2022 18:38

Dschoordsch reviewed Mar 18, 2022

View reviewed changes

Dschoordsch requested changes Mar 18, 2022

View reviewed changes

add comments to dev.js

62f929c

mattkrick merged commit fbc23f5 into master Mar 21, 2022

mattkrick deleted the feat/4754/pingpongwallawallabingbong branch March 21, 2022 17:16

adaniels-parabol mentioned this pull request Mar 25, 2022

Release test 6.51.0 #6276

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More typesafety for GraphQL code #6167

More typesafety for GraphQL code #6167

mattkrick commented Mar 4, 2022 •

edited

mattkrick Mar 4, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

mattkrick Mar 4, 2022

mattkrick Mar 4, 2022

mattkrick Mar 4, 2022

mattkrick Mar 4, 2022

mattkrick Mar 4, 2022

mattkrick Mar 15, 2022

mattkrick Mar 18, 2022

blimmer left a comment

blimmer Mar 15, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

Dschoordsch left a comment

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

Dschoordsch Mar 18, 2022

mattkrick Mar 21, 2022

mattkrick commented Mar 21, 2022


		const schema = mergeSchemas({schemas: [legacySchema], typeDefs, resolvers: resolverMap})

		const updateSchema = async () => {

		@@ -0,0 +1,7 @@
		import {QueryResolvers} from '../resolvers/types'

		export interface PingPayload {}

		@@ -0,0 +1,10 @@
		union PingPayload = ErrorPayload \| PingSuccess

		@@ -0,0 +1,48 @@
		import getRethink from '../../../../database/rethinkDriver'


		const logins: QueryResolvers['logins'] = async (_source, args) => args

		export default logins

More typesafety for GraphQL code #6167

More typesafety for GraphQL code #6167

Conversation

mattkrick commented Mar 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dschoordsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick commented Mar 21, 2022

mattkrick commented Mar 4, 2022 •

edited