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

feat: add support for metadata and persisted operations in client preset #8757

Merged
merged 15 commits into from
Jan 18, 2023

Conversation

n1ru4l
Copy link
Collaborator

@n1ru4l n1ru4l commented Dec 21, 2022

Closes #8756
Closes #8822

This implementation is very ugly IMHO, but I could not see a different way in how things are structured right now. 馃ゲ

@changeset-bot
Copy link

changeset-bot bot commented Dec 21, 2022

馃 Changeset detected

Latest commit: 7633dfa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/client-preset Minor

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

@n1ru4l n1ru4l marked this pull request as draft December 21, 2022 09:41
@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2022

馃殌 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-cli/codegen 2.4.25-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/cli 2.16.5-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/visitor-plugin-common 2.13.8-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/typescript-document-nodes 2.3.13-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/gql-tag-operations 1.6.2-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/typescript-operations 2.5.13-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/typescript-resolvers 2.7.13-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/typed-document-node 2.3.13-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/typescript 2.8.8-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/client-preset 1.3.0-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌
@graphql-codegen/graphql-modules-preset 2.5.12-alpha-20230118075831-1d93c9826 npm 鈫楋笌 unpkg 鈫楋笌

@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2022

馃殌 Website Preview

The latest changes to the website are available as preview in: https://30ddff48.graphql-code-generator.pages.dev

@shmax
Copy link
Contributor

shmax commented Jan 2, 2023

@n1ru4l I've spent the last few days trying to figure out how to get persisted queries working with codegen, myself, and I stumbled across this PR. One thing isn't clear to me: does this actually transform the client code for production builds? I don't use Apollo or Urql (I'm stubborn, but weakening), so all I really want to see is for this kind of transformation to happen...

from (dev):

const skuQuery = graphql('query productDetailsPage($id: ID) {
  sku(id: $id) {
    id
  }'
);

to (prod):

const skuQuery = {
  hash: `e3423f1c`
};

Hope that makes some sense. Please let me know what you think.

@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from b8972ae to 7d84b09 Compare January 5, 2023 19:46
@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Jan 5, 2023

@shmax Yes, this is possible 馃帀

export const CFragmentDoc = {"__hash":"e8d0fab85de43b2634f10801ec2a3f94a2850dc1"} as unknown as DocumentNode<CFragment, unknown>;
export const ADocument = {"__hash":"b61b879c1eb0040bce65d70c8adfb1ae9360f52f"} as unknown as DocumentNode<AQuery, AQueryVariables>;
export const BDocument = {"__hash":"c3ea9f3f937d47d72c70055ea55c7cf88a35e608"} as unknown as DocumentNode<BQuery, BQueryVariables>;"

@n1ru4l n1ru4l marked this pull request as ready for review January 5, 2023 21:08
@shmax
Copy link
Contributor

shmax commented Jan 6, 2023

Yes, this is possible

Ah, very cool. Okay, so that's the graphql.ts file, but what about calls to graphql (with query strings) in the source files?

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Jan 6, 2023

@shmax The graphql function returns the document node that has the hash included 馃槑

@shmax
Copy link
Contributor

shmax commented Jan 6, 2023

Sure, I understand that, but in my project I type queries right into the source, eg:

// someclientfile.ts

const ProductDetails: FC<ProductDetailsProps> = (_props) => {
  const { data } = useQuery(["sku"], async () =>
    graphqlAsync(
      graphql(/* GraphQL */ `
        query productDetailsPage($skuId: ID, $loadMod: Boolean) {
          sku(id: $skuId, loadMod: $loadMod) {
            id
            fullName
          }
        }
      `),
      {
        skuId: (9).toString(),
      }
    )
  );

   ...
}

That's the whole point of this package, isn't it? So you can generate typings for the response document? Do you have some alternate approach that hasn't occurred to me?

plugin: async () => {
await tdnFinished.promise;
return {
content: JSON.stringify(Object.fromEntries(persistedOperations.entries()), null, 2),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can use fast json here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is fast json?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch 3 times, most recently from 5b53828 to cef8337 Compare January 9, 2023 07:51
@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from d5f618c to a9062d9 Compare January 16, 2023 11:05
@n1ru4l n1ru4l changed the title feat: add support for persisted operations feat: add support for meta/ persisted operations in client preset Jan 16, 2023
@n1ru4l n1ru4l changed the title feat: add support for meta/ persisted operations in client preset feat: add support for metadata and persisted operations in client preset Jan 16, 2023
Comment on lines 233 to 234
unstable_onDocumentNode?: Unstable_OnDocumentNode;
unstable_omitDefinitions?: boolean;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added them as unstable as I don't want anyone outside the client preset to use these features tbh.

@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from 8f4f163 to 03e64aa Compare January 16, 2023 11:15
@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from 1de4db4 to 95efaf7 Compare January 16, 2023 11:54
@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from 5552d9b to 36baf2e Compare January 16, 2023 12:39
@github-actions
Copy link
Contributor

diff --git a/website/algolia-lockfile.json b/website/algolia-lockfile.json%0Aindex 104e2f2cf..3cbb7cbab 100644%0A--- a/website/algolia-lockfile.json%0A+++ b/website/algolia-lockfile.json%0A@@ -880,7 +880,7 @@%0A         "anchor": "appendix-ii-compatibility"%0A       }%0A     ],%0A-    "content": "066e74d7d5984a87318ec6661c2ffb3b",%0A+    "content": "93a7f1749a22be8fad86f9bdceee42d6",%0A     "url": "https://www.the-guild.dev/graphql/codegen/docs/guides/react-vue",%0A     "domain": "https://www.the-guild.dev/graphql/codegen/",%0A     "hierarchy": [

@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from 8717b93 to 492825d Compare January 16, 2023 12:56
Copy link
Collaborator

@saihaj saihaj left a comment

Choose a reason for hiding this comment

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

I guess we can wait for @graphql-tools/documents to publish then merge this?

@saihaj
Copy link
Collaborator

saihaj commented Jan 17, 2023

@n1ru4l n1ru4l force-pushed the feat-client-persisted-operations branch from df4d080 to d053e8c Compare January 18, 2023 07:55
@saihaj
Copy link
Collaborator

saihaj commented Jan 21, 2023

/theguild newsletter

@shmax
Copy link
Contributor

shmax commented Feb 6, 2023

@n1ru4l hey there, I've had a chance to tinker with this awesome feature a bit, and I have some feedback for you.

Invalid AST?

When persistedDocuments is turned on, we wind up with this kind of structure in our graphql.ts file:

export const UpdateWishlistMutationDocument = {
  __meta__: { hash: "c3cc8d68aaaa31edda041890c6444d412ab72208" },
  kind: "Document",
  definitions: [

Interesting, but this AST structure doesn't validate. The library I use (graphql-php) throws an exception here ("Node is missing kind").

Behavior of replaceDocumentWithHash

I tried switching on this flag, and the objects in graphql.ts reduce down to just the hash information, which is great, but why isn't this the default behavior? Why do I have to opt-in when the __meta__ node isn't validating (in at least one popular GraphQL implementation), and when one of the key justifications for using persisted queries is to reduce the amount of information being sent along with back end requests? What use could a person interested in persisted queries have for the entire AST along with the hash?

Purpose and implementation of the onExecutableDocumentNode feature

It's not clear to me what the purpose of this feature is. I originally thought it was a hook for me to transform a document node to my own taste, but it seems all it does is allow one to add a little additional payload to the __meta__ node. After examining the samples I'm not sure what the use case is, or what it has to do with persisted queries. I'm also curious about the decision to add a callback to the codegen.ts file. It's TypeScript, so it's valid, but what happens if a user is using a .yml file, instead? Aren't the two meant to be functionally equivalent?

There are still long graphql queries left in the code.

I don't know if this is, strictly speaking, part of persisted queries, but I think a lot of people are interested in replacing the queries with hashes all throughout the code (and not just in graphql.ts). I was asking about this on the original PR, but you didn't really resolve the question. This results in smaller bundle sizes, and more private code (I don't need to advertise to outside eyes details about my schema, or even the fact that I use GraphQL).

Why does persisted-documents.json contain query strings instead of ASTs? What about fragments?

You took ASTs out of graphql.ts and replaced them with hash keys, but the values in the generated persisted-documents.json are the original query strings. I guess it's not a huge problem, but what do we do with fragments? They appear in the json file with their own hashes, but they're not cross-referenced anywhere, and it's not clear to me how one would assemble all the pieces on the back end.

So, I guess I'm curious about the background of this feature. Is it inspired by something you saw in a different project? What about the design of the persisted-documents.json file? Is that format compatible with something like Apollo Server, for example? Do fragments work for you?

Is there room for an alternate approach?

Maybe I should also mention that I've prepared my own solution that manages to avoid many of the problems described above. I do it all as a custom babel plugin, which I configure in my webpack config file:

// webpack.prod.ts
...

module: {
    rules: [
      {
        test: /\.ts|tsx?$/,
        exclude: /node_modules/,
        loader: "babel-loader",
        options: {
          plugins: [
            [
              persistentQueries,
              {
                strip: true,
                parser: graphql,
                onlyMatchImportSuffix: true,
                transform: (source, ast) => {
                  const h = hash(source);
                  graphqlAstHashes[h] = ast;
                  return {
                    queryId: h,
                  };
                },
              } as PersistentQueriesConfig,
            ],
          ],
        },
      },

When the transformation is complete I write out my own file, queries.json. Because I fetch the ASTs with calls to graphql (the one from gql/graphql.ts), the fragments are all nicely resolved, and I can just write them directly to my json file and the queries run without issue once they're collected on the back end.

Furthermore, the transform callback allows me to transpile the source code from this:

const { data, isLoading, isFetching } = useQuery(
    ["sku", params.skuId, isPreview],
    async () => {
      return graphqlAsync(
        graphql(/* GraphQL */ `
          query productDetailsPage($skuId: ID, $loadMod: Boolean) {
            loggedIn
            isModerator
            skuSubmission(skuId: $skuId) {
            ...

to this:

const { data, isLoading, isFetching } = useQuery(
    ["sku", params.skuId, isPreview],
    async () => {
      return graphqlAsync({
        queryId: 'ecf6bfcf1eb9ea5dfa42954795940140072b30748cb59be8274b32c2ef88003a'
       }...

While I'm at it, I also remove any imports of graphql from the source files, so to me it doesn't matter what's in it, or how big it gets--it never makes it into the bundle.

I was planning on putting my plugin up on github somewhere, but I wasn't sure if it might be more appropriate to add it to this repo, or if it would conflict with the work you've done here. I'd be grateful for your thoughts, and those of the other contributors.

Sorry for the long post. Thanks for reading, and there's no rush on responding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants