graphql-code-generator icon indicating copy to clipboard operation
graphql-code-generator copied to clipboard

Feat: Hasura allow list global fragments support

Open BenoitRanque opened this issue 4 years ago • 11 comments

Description

Added support for globally defined fragments for the hasura-allow-list plugin. Previously it was required that fragments be defined in the same document as the operations that use them. Now, with the global_fragments config set, fragments will be pulled from all documents in the project.

Because this configuration is off by default, this feature does not break previous behavior and is thus not a breaking change.

Related #7700

Type of change

  • [x] New feature (non-breaking change which adds functionality)
  • [x] This change requires a documentation update

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?

Added tests to test suite for the plugin, testing success case and two error cases. Also built the plugin and tested on a local project to see the desired output.

Test Environment:

  • OS: windows
  • @graphql-codegen/cli: 2.6.0
  • NodeJS: 14.17.6

Checklist:

  • [x] I have followed the CONTRIBUTING doc and the style guidelines of this project
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [x] Any dependent changes have been merged and published in downstream modules

BenoitRanque avatar Mar 28 '22 21:03 BenoitRanque

🦋 Changeset detected

Latest commit: 1a9a49ece8cb9fb352d14375e407292d9cb7cc9a

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/typescript-graphql-request Patch

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

changeset-bot[bot] avatar Mar 28 '22 21:03 changeset-bot[bot]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/theguild/graphql-code-generator/9hKUFE4767ctZ3mxW8rnYSBvV4gF
✅ Preview: Failed

vercel[bot] avatar Mar 28 '22 21:03 vercel[bot]

Realizing now I pushed to the branch I initially created the plugin with. Unsure if this is an issue. Please let me know if there's any action needed on my part, including closing this and reopening under a dedicated branch

BenoitRanque avatar Mar 28 '22 21:03 BenoitRanque

Hi @BenoitRanque

You might need to rebase your PR 👀

charlypoly avatar Apr 05 '22 09:04 charlypoly

@BenoitRanque if youre still pushing this in I have what I think is a similar issue.

I am not knowledgable about how it works, but I have a fragment and query defined in the same file, with the query depending on the fragment:

const MY_FRAG = gql`
  fragment Merchant on merchant {
    name
  }
`

const MERCHANT_DATA = gql`
  ${MY_FRAG}
  query {
    merchant {
      ...Merchant
    }
`

When I run the graphql gen, MERCHANT_DATA cant find the fragment, I believe because it doesnt do js substitution when its inspecting.

If you think this is worth pursuing but its separate from your current PR I can open a ticket. Otherwise any tips?

ps. I chose to cut out fragments because I use a different generator plugin to make the types of the fragments:

//                            V generated type
const merchant: MerchantFragment = useFragment(
    MERCHANT_FRAGMENT,
    merchantFragment
  )

kevin-meyers avatar Apr 26 '22 15:04 kevin-meyers

@BenoitRanque is attempting to deploy a commit to the The Guild Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar May 29 '22 23:05 vercel[bot]

My bad, I missed the conversations here. I have rebased the branch, I think

BenoitRanque avatar May 29 '22 23:05 BenoitRanque

This commit history is scary, I am not convinced it was supposed to look this way, probably messed something up

BenoitRanque avatar May 29 '22 23:05 BenoitRanque

@BenoitRanque if youre still pushing this in I have what I think is a similar issue.

I am not knowledgable about how it works, but I have a fragment and query defined in the same file, with the query depending on the fragment:

const MY_FRAG = gql`
  fragment Merchant on merchant {
    name
  }
`

const MERCHANT_DATA = gql`
  ${MY_FRAG}
  query {
    merchant {
      ...Merchant
    }
`

When I run the graphql gen, MERCHANT_DATA cant find the fragment, I believe because it doesnt do js substitution when its inspecting.

If you think this is worth pursuing but its separate from your current PR I can open a ticket. Otherwise any tips?

ps. I chose to cut out fragments because I use a different generator plugin to make the types of the fragments:

//                            V generated type
const merchant: MerchantFragment = useFragment(
    MERCHANT_FRAGMENT,
    merchantFragment
  )

I think this would be covered by global fragment support. Each graphql file is considered a document. In your case, each graphql string would be considered a document. The downside being, if you reuse the same fragment name across multiple files you will run into issues. Also the generated allow list has the fragments at the bottom, I am not sure if that will matter.

BenoitRanque avatar May 29 '22 23:05 BenoitRanque

@BenoitRanque I'm afraid your rebase is incorrect since I can see some changes in this PR diff that are already present in master

I would suggest to create new branch and new PR by git cherry-pick your initial commits 👀

charlypoly avatar May 30 '22 09:05 charlypoly

Hi @BenoitRanque 👋🏼

Do you still plan to merge this improvement?

charlypoly avatar Aug 04 '22 13:08 charlypoly

@charlypoly Yes, apologies for the delay here, finally got around to it!

Closing this PR as replaced by dotansimha/graphql-code-generator#8242

BenoitRanque avatar Aug 16 '22 08:08 BenoitRanque