graphql-tools icon indicating copy to clipboard operation
graphql-tools copied to clipboard

Published 20 hours ago • verified publisher icon ardatan

allow using merge directive without keyArg argument if resolver takes 0 arguments

Open dimatillck opened this issue 3 years ago • 6 comments
trafficstars

Description

Allows using @merge directive without keyArg argument if resolver takes 0 arguments Related #4336

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)

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
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] 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 and linter rules pass locally with my changes
  • [ ] Any dependent changes have been merged and published in downstream modules

dimatillck avatar Mar 25 '22 17:03 dimatillck

🦋 Changeset detected

Latest commit: 6b91b09bd1094d5deec68e48f454432452ca0d63

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

This PR includes changesets to release 2 packages
Name Type
@graphql-tools/stitching-directives Patch
federation-benchmark 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 25 '22 17: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-tools/BNni3KjdepM6quL96r8baCU7xj7U
✅ Preview: Failed

[Deployment for 6b91b09 failed]

vercel[bot] avatar Mar 25 '22 17:03 vercel[bot]

The failed test is unrelated, please rerun it

dimatillck avatar Mar 25 '22 17:03 dimatillck

But does merging work? Do we have a test elsewhere? I think it does and we do, but offhand I am not sure where.

yaacovCR avatar Mar 27 '22 02:03 yaacovCR

Hi, @yaacovCR , I did a manual test and it works. I didn't find the test so I added it, please check it

dimatillck avatar Mar 28 '22 11:03 dimatillck

@yaacovCR well, now I can confirm that merging doesn't work well if querying not all the fields from the type. I've updated the test case to fail.

dimatillck avatar Apr 03 '22 15:04 dimatillck