-
Notifications
You must be signed in to change notification settings - Fork 10
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
Tighter integration with persisted queries and GraphQL Codegen #414
Conversation
🦋 Changeset detectedLatest commit: 3ef87a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
* | ||
* @since 1.2.0 | ||
*/ | ||
export function fromGraphQLCodegenPersistedDocuments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this functionality also be available on the CLI as an option, or should we generally rely more on the config for this kind of change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this seems fine I think.
}, | ||
}); | ||
} | ||
|
||
maybeReportErrorsAndExit(uniq(sources.map((source) => source.file))); | ||
|
||
// Using createFragmentRegistry means our minimum AC version is 3.7. We can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that we leave out the fragment registry if we've parsed our documents from a format that's supposed to have already done the fragment stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! 8243f09
* | ||
* @since 1.2.0 | ||
*/ | ||
export function fromGraphQLCodegenPersistedDocuments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this seems fine I think.
@@ -96,6 +96,27 @@ Default: | |||
] | |||
``` | |||
|
|||
#### Usage with GraphQL Codegen persisted documents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I feel like the docs could somehow be a bit clearer that there are two distinct advantages to taking this approach:
- The sorta obvious reason: if you've already carefully configured graphql-codegen to know exactly which files contain GraphQL documents and you trust it to have done so accurately, you might as well just piggy-back on its work rather than having to configure our tool identically.
- The less obvious reason: if you're using certain graphql-codegen features (like the client-preset) then the actual documented being passed to Apollo Client is not the value you wrote in your source code, but the outcome of some light transforms applied by graphql-codegen. While those transforms are intended to be similar to the ones that AC does, they aren't quite the same, so to analyze things accurately we should be running our tool against the output of that process.
I'm not really sure how to reword that in a clearer and more succinct manner but it doesn't seem like the README right now tries to express this? Though maybe it's subtle enough to be futile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya thats a good point. I'll take a stab at conveying both and see how it feels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a stab at both with b16fcf2. Let me know what you think!
export default config; | ||
``` | ||
|
||
> NOTE: Running these documents through this package is necessary to ensure all document transforms built into Apollo Client are also run on these documents. This ensures forward compatibility with any future changes introduced by Apollo Client that may alter the GraphQL document output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of explains the opposite issue to the one I'm concerned about above (that one could imagine just taking the codegen JSON and generating our manifest from it directly rather than using ApolloClient like this tool does), whereas my thought above is about "can't you just use the normal version of this tool directly on your app source".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny I think this shows the 2 different ways you and I think about this 🤣. Some of this comes form insider knowledge on whats coming with Apollo Client, but you have a solid point here. I think it might be worth expressing both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a stab at this with b16fcf2. Let me know what you think!
Co-authored-by: David Glasser <[email protected]>
GraphQL Codegen provides a persisted documents feature that outputs a manifest file with queries detected by the package. Unfortunately the output format and generated queries aren't fully compatible with
@apollo/generate-persisted-query-manifest
.This PR aims to add tighter integration with the persisted documents feature to allow us to parse and use the documents in that manifest as the source of truth. Doing so will skip file system traversal and instead rely on the documents written to that manifest.