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

Replaced merge-graphql-schemas with graphql-tools #1322

Merged
merged 9 commits into from
Jan 27, 2021

Conversation

himankpathak
Copy link
Contributor

Description

Replaced mergeType from merge-graphql-schemas with mergeTypeDefs graphql-tools

Verification Process

Tested on local with prebuilt tests

Applicable Issues

Closes #1230

@thedavidprice thedavidprice requested a review from peterp October 10, 2020 17:22
@thedavidprice
Copy link
Contributor

Awesome, thanks for getting this rolling @himankpathak 🚀

Handing you off to @peterp from here out.

@peterp
Copy link
Contributor

peterp commented Oct 11, 2020

@himankpathak I'm surprised that the tests are working locally since this doesn't seem to be building properly. Perhaps I over estimated the coverage that we have.

@himankpathak
Copy link
Contributor Author

@peterp, I am surprised too. Locally test passed. But on PR the build is failing.

@Urigo
Copy link

Urigo commented Oct 13, 2020

Just a small note - if you want to use the new GraphQL Tools, you better install the scoped packages, so you'll get only the parts of graphql-tools that you need

@peterp
Copy link
Contributor

peterp commented Oct 18, 2020

Hey @himankpathak

It looks like it's the same issue as before - I'll check out your branch in a few minutes and figure this out.

@himankpathak
Copy link
Contributor Author

That would be great! @peterp

@thedavidprice
Copy link
Contributor

@peterp is this PR + work still relevant. If so, @himankpathak are you still interested in working on this?

Thanks to you both!

@himankpathak
Copy link
Contributor Author

@thedavidprice definitely!
I will resolve the conflicts and see if we sort the failing tests.

@peterp
Copy link
Contributor

peterp commented Jan 24, 2021

Thanks @himankpathak!

@himankpathak
Copy link
Contributor Author

Test cases are passing on local

We have following error on packages/api/src/makeMergedSchema/makeMergedSchema.ts L167

No overload matches this call.
  Overload 1 of 3, '(types: (string | GraphQLSchema | Source | DocumentNode)[], config?: (Partial<Config> & { commentDescriptions: true; }) | undefined): string', gave the following error.
    Type 'Record<string, unknown>' is not assignable to type 'string | GraphQLSchema | Source | DocumentNode'.
      Type 'Record<string, unknown>' is not assignable to type 'string'.
  Overload 2 of 3, '(types: (string | GraphQLSchema | Source | DocumentNode)[], config?: Pick<Partial<Config>, "useSchemaDefinition" | "forceSchemaDefinition" | ... 4 more ... | "convertExtensions"> | undefined): DocumentNode', gave the following error.
    Type 'Record<string, unknown>' is not assignable to type 'string | GraphQLSchema | Source | DocumentNode'.
      Type 'Record<string, unknown>' is not assignable to type 'string'.ts(2769)

@github-actions
Copy link

@thedavidprice
Copy link
Contributor

@peterp I just pushed code from the Migration from Merge GraphQL Schemas guide that proxies mergeTypes to mergeTypeDefs. There was an API change — this solution was an, ahem, efficient step forward although I'm sure it could be condensed.

Tested on my local project and everything is working fine (note the need to account for this change to test a project). Tests passed locally.

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@peterp
Copy link
Contributor

peterp commented Jan 27, 2021

Before we release v0.24.0 I would like to try this out in Snaplet.

@peterp peterp added this to the next release milestone Jan 27, 2021
@peterp peterp merged commit 6d37f4f into redwoodjs:main Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace merge-graphql-schemas with graphql-tools
4 participants