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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(sdl): codemod it all #6228

Merged
merged 29 commits into from
Mar 24, 2022
Merged

chore(sdl): codemod it all #6228

merged 29 commits into from
Mar 24, 2022

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Mar 18, 2022

This moves graphql/intranetSchema to graphql/private.

It refactors all the objects to SDL-driven development.
It's big, but half of the changes are deletions. The other half are generated, no need to read through the resolve functions, they haven't changed a bit (except for the type errors that this caught 馃帀 )

You can learn more here: https://www.loom.com/share/10ffe21d92014cb6b82f7d8aeb435516

TEST

  • Goto /admin/graphql and query something on the private schema. it works

@mattkrick mattkrick marked this pull request as ready for review March 18, 2022 23:31
reject(e)
})
})
return {newFeature}
Copy link
Member Author

@mattkrick mattkrick Mar 18, 2022

Choose a reason for hiding this comment

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

already paying off! the old resolve function wasn't returning anything and the new type caught the error 馃帀

return collector
}

const resolverMap: Resolvers = {
Copy link
Member Author

Choose a reason for hiding this comment

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

we'll never have to touch this file again, everything is automatically imported 馃帀

path.join(__PROJECT_ROOT__, 'packages/server/graphql/private/typeDefs/*.graphql')
)

const schema = makeExecutableSchema({typeDefs, resolvers})
Copy link
Member Author

Choose a reason for hiding this comment

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

we'll never have to touch this file again, everything is automatically imported 馃帀

@@ -0,0 +1,15 @@
import {DraftEnterpriseInvoicePayloadResolvers} from '../resolverTypes'

export type DraftEnterpriseInvoicePayloadSource =
Copy link
Member Author

Choose a reason for hiding this comment

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

this type has to be manually added & then we have to add it into codegen.json, which isn't great, but it already caught some more type errors

@@ -0,0 +1,166 @@
/*
Usage: jscodeshift --extensions=tsx,ts,js --parser=tsx -t ./scripts/codeshift/extractResolver.ts ./packages/server/graphql/intranetSchema/queries
Copy link
Member Author

Choose a reason for hiding this comment

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

i walk through this in the loom video.
it's kinda fun if you like metaprogramming

scripts/codeshift/extractResolver.ts Outdated Show resolved Hide resolved
@mattkrick mattkrick mentioned this pull request Mar 21, 2022
1 task
@mattkrick mattkrick changed the base branch from master to 5385/template-illustrations March 21, 2022 17:17
@mattkrick mattkrick changed the base branch from 5385/template-illustrations to master March 21, 2022 17:18
@mattkrick
Copy link
Member Author

mattkrick commented Mar 21, 2022

@Dschoordsch I experimented with a default mapper being void or never in order to force devs to explicitly write a *Source type definition. Here's my takeaways:

The current behavior has a default type == the GraphQL type. This works great for the following cases:

  • The return type is a primitive. no mapper is necessary.
  • The Source type is a superset of the GraphQL type. no mapper is necessary.

Now the cool part is when the Source type is a subset of the GraphQL type, typescript still throws an error.

A good example is user.ts:

  • the user query returns the type IUser, which comes from postgres
  • the user query is expected to return the typescript equivalent of /graphql/types/User.ts
  • Therefore, we get an error because e.g. IUser does not include the field timeline.
  • to play around with it, remove the User mapper & recompile to see the error showing up

This error forces the developer to add an entry to the codegen#mappers list. there's no implicit any in the logic chain 馃帀

@mattkrick mattkrick requested a review from JimmyLv March 21, 2022 20:43
@mattkrick
Copy link
Member Author

@JimmyLv thought this would be a fun PR for you to review! it's mainly a refactor to make our feature building a little more modern

@@ -0,0 +1,87 @@
import {InvoiceItemType, SubscriptionChannel} from 'parabol-client/types/constEnums'
import adjustUserCount from '../../../billing/helpers/adjustUserCount'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider using a typescript path to make these ../../../ imports simpler to read?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, absolutely. in some places we use ~ to mean parabol-client, and in other places we use symlinks via lerna to get to different packages (e.g. parabol-server, parabol-client) but getting vscode, webpack, and typescript to all suggest the same pattern has been a bear. We need to take a fresh look at our configs because there's a devil in one of them...

@@ -1,4 +1,5 @@
module.exports = {
// "*.{ts,tsx}": 'organize-imports-cli',
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, there's a prettier plugin that does this for you if you want to enable it again someday: https://github.com/simonhaenisch/prettier-plugin-organize-imports#readme

Copy link
Member Author

Choose a reason for hiding this comment

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

ahhh i've gotta try that next time! this one was buggy & slow

Copy link
Contributor

@blimmer blimmer left a comment

Choose a reason for hiding this comment

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

I didn't review each individual resolver, but this is a great step to further leveraging the modern GraphQL ecosystem!

@mattkrick
Copy link
Member Author

@JimmyLv hows the review going? any questions or comments?

@mattkrick mattkrick added this to To Prioritize in Sprint Board via automation Mar 24, 2022
@mattkrick mattkrick merged commit a71b95a into master Mar 24, 2022
@mattkrick mattkrick deleted the chore/jscodeshift branch March 24, 2022 03:48
Dschoordsch added a commit that referenced this pull request Mar 24, 2022
This reverts commit a71b95a.

The commit breaks the internal resolver.
@adaniels-parabol adaniels-parabol mentioned this pull request Mar 25, 2022
16 tasks
@JimmyLv
Copy link
Contributor

JimmyLv commented Mar 25, 2022

@mattkrick Hey Matt! This is really excellent! I watched the video earlier, to understand the intent behind this.

This is also the first time, I learned how to write a jscodeshift script, understand the magic behind it!
As you mentioned in the loom video, will we move on to the public schema next? what and how different are these folders of public stuff?

@mattkrick
Copy link
Member Author

yep, next step is the public schema!
the public schema is about 20x larger than this private schema, so that's why i wanted to make this private schema patterns perfect before we tackle the MASSIVE amount of work in the public schema

@JimmyLv
Copy link
Contributor

JimmyLv commented Mar 30, 2022

Makes sense! I believe the jscodeshift for private schema could be adapted to public schema, it would be a lifesaver!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Sprint Board
  
To Prioritize
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants