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

chore(SDL): refactor _legacy.graphql into individual typeDefs #10019

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Jul 23, 2024

Description

Refactoring into the SDL pattern has been a pain because we need to remove the typeDef from the _legacy.graphql file & put it into its own typeDef. We do this because it's 8000 LOC & ripe for merge conflicts.

I wrote some code that did this for us for every type.
Now refactoring to the SDL pattern just requires transforming the resolve statements, no need to touch .graphql files 🎉

How is it broken out?

  • Every type has its own file, even one-liners like unions & scalars
  • Mutations & Queries are types too, so you'll see a Mutation.graphql
  • Additionally, we have _xGitHub and _xGitLab.graphql, which override their integrations

Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

@mattkrick mattkrick requested a review from nickoferrall July 23, 2024 22:31
@@ -0,0 +1,2018 @@
type Mutation {
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked the previous way it was split into multiple files better: put each mutation in its own file and add the related return and input types there too. The second part of this might be a bit harder to automate, but then you have all related types in one file and don't need to jump between them too often.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was definitely nice grouping them together & having fewer files!
The question I kept asking myself is, "why are we treating the Mutation object differently from other objects?"

If the mutation returned a User object, we wouldn't put the User definition in there, we'd give it its own file because it's shared across definitions.
Using that same logic, UpdateMeetingPromptSuccess type is shared with the Subscription object, so why does that get grouped with in the same file as the mutation field?

Moving forward, I'm down to keep using out extend pattern! TBH I don't really care where the defs are stored, I just use search anyways. I just wanted to get rid of the stale definitions in _legacy.graphql that were holding us back.

How should we proceed? Could we merge this now, or should we refactor manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just a preference. I don't see a reason why we shouldn't merge it as is. Better than manually moving types out of _legacy.graphql.

Copy link
Contributor

@nickoferrall nickoferrall left a comment

Choose a reason for hiding this comment

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

Love this!

@mattkrick mattkrick merged commit f8b029d into master Jul 25, 2024
6 checks passed
@mattkrick mattkrick deleted the schema-defs branch July 25, 2024 17:35
@github-actions github-actions bot mentioned this pull request Jul 26, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants