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

Submitted on behalf of a third-party: add mergeResolvers #1084

Closed

Conversation

finnigantime
Copy link

@finnigantime finnigantime commented Mar 26, 2019

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

Implements #1083.

This PR takes an existing well-publicized gist https://gist.github.com/hellendag/2aa9ad1f9b771f38802760c269bb1b76 (with two small Typescript typing changes) and exposes it as part of graphql-tools. Currently we copy-pasted the gist into our codebase but we don't want to have to maintain it ourselves and keep it in sync with the rest of graphql/graphql-tools. It is generally useful for schema mocking and makes sense to expose in the graphql-tools lib.

@apollo-cla
Copy link

@finnigantime: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

| { [key: string]: ResolvedValue | ResolverFunction }
| null
| ResolvedValue
| ResolverMap;
Copy link
Author

Choose a reason for hiding this comment

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

the two typing changes from the original gist are to add | ResolvedValue | ResolverMap here.

A ResolverMap value can be a ResolvedValue directly - no need for a ResolverFunction. This seems to work fine in our testing.

The other change is to allow a ResolverMap value to be another ResolverMap. The ResolverMap can be multilevel. For instance, one test we wrote passes:

    const client = mockClient({
      WorkItem: () => ({
        queue: () => ({
          id: () => 'test-queue-id-1',
        }),
      }),
    });

(here, mockClient is our local mock Apollo client and it calls mergeResolvers() with the first argument being our base set of mock resolvers for all tests, and the second argument being the test-specific resolver override map)

@finnigantime finnigantime changed the title add mergeResolvers Submitted on behalf of a third-party: add mergeResolvers Mar 26, 2019
@rhoskal
Copy link

rhoskal commented Jun 17, 2019

Just throwing out a gentle bump here... what's the status of this PR?

@finnigantime
Copy link
Author

I would love to get this in. On my end, this is ready to merge. We left a note to @hellendag on the gist to notify. Repo admins - ok to merge this?

@crisu83
Copy link
Contributor

crisu83 commented Oct 17, 2019

@finnigantime Why are there no unit tests for mergeResolvers?

@smeevil
Copy link

smeevil commented Jan 14, 2020

Quite interested in this and wondering what is blocking it.

@finnigantime
Copy link
Author

Nothing from my end

@yaacovCR
Copy link
Collaborator

Wondering about adding to graphql-tools-fork. What does this do that mergeDeep does not?

@yaacovCR
Copy link
Collaborator

I see, this runs both resolvers to make sure to return something.

@hwillson hwillson removed their request for review April 8, 2020 11:54
@ardatan
Copy link
Owner

ardatan commented Apr 23, 2020

Thank you @finnigantime for this contribution ! We are planning to merge your code with mergeResolvers from @graphql-toolkit/schema-merging.

@ardatan ardatan closed this Apr 23, 2020
@tamlyn
Copy link

tamlyn commented Aug 16, 2022

@graphql-toolkit/schema-merging has become @graphql-tools/merge but the above mock merging function code still hasn't been included AFAICT.

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

Successfully merging this pull request may close these issues.

None yet

8 participants