feat: Add stripIgnoredCharacters option (visitor-plugin-common) - continued #8489
Conversation
🦋 Changeset detectedLatest commit: 37d7dba The changes in this PR will be included in the next version bump. This PR includes changesets to release 46 packages
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 |
|
Hi @JanStevens, Thank you for continuing @aradalvand's work here. Considering our existing plan to address the issues linked to the amount of generated code, we don't plan to add this feature to Also, we don't judge this issue as blocking since:
|
|
Hi @charlypoly, Thanks for the swift answer, v3 sounds great! Concerning committing generated files: It's a bit hard to have a working typescript setup in various stages of CI / local development. Especially if those types are changing a lot, it could introduce typescript errors when performing production deploys so we rather have them committed. Down the line I think that using the I'll take a look at the Regards |
|
@charlypoly This is not about the types though, it's about the strings, strings aren't wiped out in the compliation step like types are (obviously) and so I don't see how your bullet points apply to this PR. Correct me if I'm wrong. |
My reply was not concerning this PR as it can be closed, but rather at the comment from @charlypoly regarding V3 ;) |
|
My bad, I meant to mention @charlypoly... Made a mistake. I don't think this PR should've been closed! We need this option anyway. |
No worries, by all means reopen it 😉 |
@JanStevens I can't, you're the author :) I think you should keep it open unless @charlypoly decides otherwise. In this comment of mine I was trying to point out, in response to @charlypoly, that I'm not sure how his points were relevant to this particular feature, this PR/feature is concerned with the strings, and not the types; unless I've misunderstood something here. @charlypoly Would love for you to clarify. |
Sure! const documents = {
"\n query allFilmsWithVariablesQuery($first: Int!) {\n allFilms(first: $first) {\n edges {\n node {\n ...FilmItem\n }\n }\n }\n }\n": types.AllFilmsWithVariablesQueryDocument,
"\n fragment FilmItem on Film {\n id\n title\n releaseDate\n producers\n }\n": types.FilmItemFragmentDoc,
};it heavily relies on TypeScript function overloads on the provided const documents = {
"\n query allFilmsWithVariablesQuery($first: Int!) {\n allFilms(first: $first) {\n edges {\n node {\n ...FilmItem\n }\n }\n }\n }\n": types.AllFilmsWithVariablesQueryDocument,
"\n fragment FilmItem on Film {\n id\n title\n releaseDate\n producers\n }\n": types.FilmItemFragmentDoc,
};
export function graphql(source: "\n query allFilmsWithVariablesQuery($first: Int!) {\n allFilms(first: $first) {\n edges {\n node {\n ...FilmItem\n }\n }\n }\n }\n"): (typeof documents)["\n query allFilmsWithVariablesQuery($first: Int!) {\n allFilms(first: $first) {\n edges {\n node {\n ...FilmItem\n }\n }\n }\n }\n"];
export function graphql(source: "\n fragment FilmItem on Film {\n id\n title\n releaseDate\n producers\n }\n"): (typeof documents)["\n fragment FilmItem on Film {\n id\n title\n releaseDate\n producers\n }\n"];
export function graphql(source: string): unknown;
export function graphql(source: string) {
return (documents as any)[source] ?? {};
}As you might guess, stripping all ignored characters will break the TS function overloading here. While this PR still applies to existing plugins, we are right now focusing our efforts on bringing new features to the v3 roadmap and fixing blockers in existing versions. |
|
You could still use I'm looking forward for the new approach, right now I can't really grasp how to use this with for example |
Yes but then your GraphQL Operations would need to be written in a condensed way at the source too.
You will find a complete example here: https://github.com/dotansimha/graphql-code-generator/tree/master/examples/front-end/react/graphql-request and new documentation here: https://www.the-guild.dev/graphql/codegen/docs/guides/react-vue |
Cool, thanks for the docs. We currently have a setup with |
|
I can't wrap my head around this supposed v3 |
The
The In short, we use the literal GraphQL definition strings to build function overloads on |
|
@charlypoly Hi, it seems like v3 has been released and this aspect of the library hasn't really changed, so I think this PR is still relevant. |
|
@charlypoly Could you please respond? |
Description
Adds a new stripIgnoredCharacters: boolean option to client-side-base-visitor that removes redundant characters (like line-breaks, whitespaces, etc.) from the query strings.
In the linked issue, I originally proposed doing .replace(/\s+/g, ' ').trim() on the strings, but that turned out to be an unreliable approach as it would fail to cover a number of cases:
query ( $var : Int! )(should becomequery ($var: Int!)) or more commonlyquery { foo bar }(should becomequery{foo bar}).This all means a GraphQL-specific minifier is actually needed to do this reliably,
Happily I found out there's a built-in function in graphql-js for this very task! stripIgnoredCharacters — see graphql/graphql-js#1523, which does precisely this.
Related # (issue)
Related to #5616 and continuation of #8322
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
ts-documents.spec.ts, but I'm not really sure this is the best location. I looked at how other options are tested but it seems there are barely any tests for the client visitor.Test Environment:
@graphql-codegen/...: latestChecklist:
Further comments
This is the continuation of PR #8322 all credits go to @aradalvand