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

fix(mergeSchemas): Prevent custom resolvers from being overwritten by default resolvers #4368

Closed
wants to merge 1 commit into from

Conversation

mattkrick
Copy link

@mattkrick mattkrick commented Apr 6, 2022

Description

custom resolvers should overwrite default resolvers

Related #4367

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

  1. Do a merge schema, which will give every field a default resolver
  2. Do a merge schema again with custom resolvers
  3. Notice the custom resolver is being used (below you should see '42' in the console)
const mySchema = mergeSchemas(...) // this will add a default resolver to every field
const withNestedSchema = mergeSchemas({
  schemas: [mySchema],
  resolvers: {_extQuery: { projects: () => console.log(42); return 42}} // this resolver gets overwritten during merge!
})

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2022

⚠️ No Changeset found

Latest commit: 61c5221

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Apr 6, 2022

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/7WDnpFqtQFQjsGaCVJCVAQxUQ49R
✅ Preview: Failed

@borisno2
Copy link

Hey @mattkrick just looking at the PR, it looks like it hasn't been touched in a while, is this something you are still working on?

@ardatan if this passes tests is this something you would be happy to merge?

I am happy to help get this one over the line if required. It would help us with an issue we are having over at KeystoneJS keystonejs/keystone#7527

Thanks!

@@ -30,7 +30,8 @@ export function mergeSchemas(config: MergeSchemasConfig) {
extractedResolvers.push(getResolversFromSchema(schema));
extractedSchemaExtensions.push(extractExtensionsFromSchema(schema));
}

extractedResolvers.push(asArray(config.resolvers || []))

Choose a reason for hiding this comment

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

Using the following here, resolved the type errors in the tests for me

 const resolvers = config.resolvers || [];
  for (const resolver of asArray(resolvers)) {
    extractedResolvers.push(resolver);
  }

@@ -30,7 +30,8 @@ export function mergeSchemas(config: MergeSchemasConfig) {
extractedResolvers.push(getResolversFromSchema(schema));
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of the change above, we can basically change this push tounshift

Copy link

@borisno2 borisno2 May 12, 2022

Choose a reason for hiding this comment

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

Hi @ardatan
Yes, this looks to work from my testing today...

I don't have access to make this change, @mattkrick is this a change you are able to make?

@ardatan if @mattkrick is not able to do this, is this something you are able to push, or would you like me to create a new PR with this change and the changeset?

Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

We'd love to accept a new PR :)

Choose a reason for hiding this comment

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

Thanks @ardatan - I have created #4455

@ardatan
Copy link
Owner

ardatan commented May 11, 2022

I'm afraid this can be a breaking change for the existing users relying on this behavior.
So is it possible to make sure we won't have regressions later?
And also could you add a changeset with yarn changeset?

@ardatan
Copy link
Owner

ardatan commented May 17, 2022

Closed in favor of #4455

@ardatan ardatan closed this May 17, 2022
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

3 participants