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

refactor: significantly clean up the federation.gotpl template #3187

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

clayne11
Copy link
Contributor

@clayne11 clayne11 commented Jul 17, 2024

There were a number of inline structs and inline functions that made it extremely hard to reason about what the code is doing. Split these out into smaller functions with less closures and mutation.

Part of the setup to help with fixing #2991.

There should be no behavior changes in this PR, simply refactoring code.

I also added a new package _examples/all to make it easier to debug Federation subgraphs.

Tested by running this query in the _example/federation demo before and after my changes:

query {
  topProducts {
    reviews {
      author {
        id
        reviews {
          body
        }
      }
    }
  }
}

Some configurations weren't working due to a missing resolver.
@clayne11 clayne11 force-pushed the curtis.layne/fed-refactor branch 3 times, most recently from 2ca899a to 6b2c54a Compare July 18, 2024 00:09
@coveralls
Copy link

coveralls commented Jul 18, 2024

Coverage Status

coverage: 74.714% (+0.01%) from 74.704%
when pulling 7eebd7f on clayne11:curtis.layne/fed-refactor
into a63f94b on 99designs:master.

@clayne11 clayne11 force-pushed the curtis.layne/fed-refactor branch 3 times, most recently from c4b74f9 to f8f648b Compare July 18, 2024 00:22
This enables engineers to more easily run the debugger on the Federation example. Updated README to show how to use it.
There were a number of inline structs and inline functions that made it extremely hard to reason about what the code is doing. Split these out into smaller functions with less closures and mutation.

Part of the setup to help with fixing 99designs#2991.
@@ -5,6 +5,7 @@
"main": "gateway/index.js",
"type": "module",
"scripts": {
"start-gateway": "node gateway/index.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Saves me time.

@@ -1,3 +1,14 @@
### Federation

[Read the docs](https://gqlgen.com/recipes/federation/)

## Testing
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a huge fan of making it easier for people besides me to debug through this 😆

Copy link
Collaborator

@StevenACoffman StevenACoffman left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I would love to move things from federation.gotpl to federation.go wherever possible. Some of the static analysis tooling is defeated when logic is hidden in the federation.gotpl, and it's harder to read and maintain inside a template.

The only thing about this PR that gives me pause is that it doesn't improve things in that regard, but it maybe that is in your future plans.

@StevenACoffman StevenACoffman merged commit cd82be0 into 99designs:master Jul 18, 2024
17 checks passed
@StevenACoffman StevenACoffman added the federation Related to Apollo federation label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
federation Related to Apollo federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants