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

RFC: Add a way to minify query before sending it #3073

Open
LoMonacoSalvatore opened this issue Mar 17, 2023 · 8 comments
Open

RFC: Add a way to minify query before sending it #3073

LoMonacoSalvatore opened this issue Mar 17, 2023 · 8 comments
Labels
documentation 📖 This needs to be documented but won't need code changes

Comments

@LoMonacoSalvatore
Copy link

Summary

I'm trying to send a query to Contentful GraphQL API, which has a query limit of 8Kb.
The query in question is slightly above the limit.
I've tried to minify it by using https://www.npmjs.com/package/graphql-query-compress this package, and by compressing the query string I've managed to get it to around 4Kb.

After feeding the Urql client the query, it seems like it decompresses it somehow before sending it to the API, since I still see that error in the response.

Is it possible to add a compressQuery option before sending it?

Not sure if this counts as an issue or a question or an RFC, be free to scold me if it's not an RFC :)

Thanks for your time reading this.

Proposed Solution

Add an option to compress/minify the query string before sending it

@LoMonacoSalvatore LoMonacoSalvatore added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Mar 17, 2023
@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Mar 17, 2023

You can already do this yourself by authoring an exchange that would sit in front of the fetchExchange

EDIT: oh, no it looks like our fetch itself explodes it again before sending it to the server so the minification should happen there https://github.com/urql-graphql/urql/blob/%40urql/core%403.2.2/packages/core/src/internal/fetchOptions.ts#L26

@LoMonacoSalvatore
Copy link
Author

So possibly going from this
query: stringifyDocument(request.query)
to this
query: minifyQuery ? compress(stringifyDocument(request.query)) : stringifyDocument(request.query)

Could easily fix the issue since that's the last point where the query string gets manipulated?

Maybe this could be an easy fix for a yarn patch until you guys discuss the idea at a broader level

@JoviDeCroock
Copy link
Collaborator

From my point of view this isn't really something I would want to put into urql, I would be open to finding a way where stringifyDocument can pick up prior minified work but not adding a library that adds a lot of bundle-size

@LoMonacoSalvatore
Copy link
Author

I understand, and agree with it.

For anyone encountering this issue, before it gets resolved at a stringifyDocument level, I can confirm that patching (with yarn patch) the dist file and adding the compress function from the library stated in the comment above worked like a charm.

@JoviDeCroock if you need any help on how to preserve minification on stringifyDocument let me know.

Also, as a random thought: what if the query gets minified by default? Wouldn't it be better from a performance standpoint? My query dropped from 8Kb to 4Kb, it seems a little but sizeable improvement tbh.

Thanks again for your kind help!

@kitten
Copy link
Member

kitten commented Mar 17, 2023

I'm not sure this is something that belongs in @urql/core. To be very honest, I think making several assumptions what a GraphQL document normalizes to is in a way important.

For context, internally a GraphQL query document is always parsed, and it's a "transports" responsibility to stringify it back to a GraphQL query document string.
This is why the fetchExchange sends it in its normalized form. This is also important for Persisted Query support.

In fact, I'm very surprised that Contentful imposes a length limit, rather than a field limit. I think their length limit isn't unreasonable, but I'm surprised it's this low. For context, it's only twice as long as our rather low default GET URL length limit.

You can create a custom fetchExchange to circumvent this. We expose utilities in @urql/core/internal to enable this, e.g.:

import { filter, merge, mergeMap, pipe, share, takeUntil } from 'wonka';
import { Exchange } from '@urql/core';

import {
  makeFetchBody,
  makeFetchURL,
  makeFetchOptions,
  makeFetchSource,
} from '@urql/core/internal';

import compress from 'graphql-query-compress'

export const fetchExchange: Exchange = ({ forward }) => {
  return ops$ => {
    const sharedOps$ = share(ops$);
    const fetchResults$ = pipe(
      sharedOps$,
      filter(operation => {
        return operation.kind === 'query' || operation.kind === 'mutation';
      }),
      mergeMap(operation => {
        const body = makeFetchBody(operation);
        if (body.query)
          body.query = compress(body.query);

        return pipe(
          makeFetchSource(
            operation,
            makeFetchURL(operation, body),
            makeFetchOptions(operation, body)
          ),
          takeUntil(
            pipe(
              sharedOps$,
              filter(op => op.kind === 'teardown' && op.key === operation.key)
            )
          )
        );
      })
    );

    const forward$ = pipe(
      sharedOps$,
      filter(operation => {
        return operation.kind !== 'query' && operation.kind !== 'mutation';
      }),
      forward
    );

    return merge([fetchResults$, forward$]);
  };
};

@LoMonacoSalvatore
Copy link
Author

LoMonacoSalvatore commented Mar 17, 2023

Thank you @JoviDeCroock and @kitten!

One question tho, where should I create this custom fetchExchange?
I can't find how to use the exposed utilities in the urql doc.

@LoMonacoSalvatore
Copy link
Author

@kitten kitten added documentation 📖 This needs to be documented but won't need code changes and removed future 🔮 An enhancement or feature proposal that will be addressed after the next release labels Mar 24, 2023
@kitten
Copy link
Member

kitten commented Mar 24, 2023

Marking this as documentation, since we'll simply have to document @urql/core/internal and its fetch source utilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📖 This needs to be documented but won't need code changes
Projects
None yet
Development

No branches or pull requests

3 participants