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

feat(gatsby): use embedded remote schemas #19504

Merged
merged 1 commit into from Dec 9, 2019

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Nov 14, 2019

Avoid additional layer of delegation.

Description

Switches to graphql-tools-fork, a maintained fork of the original repository with continued support for schema stitching. The fork allows for embedding a remote schema configuration within the call to transform schema, removing an additional layer of delegation within root fields and merging by defaultMergedResolver.

avoids additional layer of delegation
@yaacovCR yaacovCR requested a review from a team as a code owner November 14, 2019 11:14
@yaacovCR
Copy link
Contributor Author

@freiksenet, what do you think?

@pieh pieh added the topic: GraphQL Related to Gatsby's GraphQL layer label Nov 14, 2019
@wardpeet wardpeet changed the title use embedded remote schemas feat(gatsby): use embedded remote schemas Nov 15, 2019
@freiksenet
Copy link
Contributor

Hello @yaacovCR!

So far it has been working with older version of graphql-tools. I wonder if you could provide motivation for moving to your fork, are there any use cases that this solves for gatsby-plugin-graphql?

Thanks!

@freiksenet freiksenet added the status: awaiting author response Additional information has been requested from the author label Nov 15, 2019
@yaacovCR
Copy link
Contributor Author

ardatan/graphql-tools#1206 (comment)

I highlighted one feature related to the use here of transformSchema, in fork you get to specify remote schema details within transform schema, so that there is only one wrapping outer schema, rather than two layers of delegation, with a speed improvement on my completely non representative test machine of about 15 ms.

There is also fixed handling of errors from remote schema that should now be passed along appropriately, not sure how that is handled within Gatsby, whether that is something that can be taken advantage of, but it might be helpful!

@yaacovCR
Copy link
Contributor Author

@freiksenet @stefanprobst

Thoughts?

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 27, 2019

@freiksenet @stefanprobst

Of course, other benefits of the fork is that it supports multiple new transforms that allow you to modify objects types and fields other than root fields.

The plugin could allow passing these options to the fork from Gatsby.

On the stitching side, the fork supports merging fields from output types with identical names from multiple subschemas, which also might be useful!

@stefanprobst
Copy link
Contributor

I'm very much in favor of switching to graphql-tools-fork -- upstream graphql-tools looks pretty much abandoned, cf ardatan/graphql-tools#1206

@KyleAMathews
Copy link
Contributor

Maybe we could rename it to graphql-cool-tools haha

@vladar vladar removed the status: awaiting author response Additional information has been requested from the author label Dec 5, 2019
@freiksenet freiksenet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 9, 2019
@gatsbybot gatsbybot merged commit 588eaf7 into gatsbyjs:master Dec 9, 2019
@gatsbot
Copy link

gatsbot bot commented Dec 9, 2019

Holy buckets, @yaacovCR — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@vladar
Copy link
Contributor

vladar commented Dec 10, 2019

We've got an issue after switching: #20015

We were using:

const {
  createResolveType,
  fieldMapToFieldConfigMap,
} = require(`graphql-tools/dist/stitching/schemaRecreation`)

Which was changed to:

const {
  createResolveType,
  fieldMapToFieldConfigMap,
} = require(`graphql-tools-fork`)

But looks like there are no exports like this in graphql-tools-fork. @yaacovCR any alternatives?

@freiksenet
Copy link
Contributor

Reverting this for now.

freiksenet added a commit that referenced this pull request Dec 10, 2019
freiksenet added a commit that referenced this pull request Dec 10, 2019
@yaacovCR
Copy link
Contributor Author

Sorry about that, the fork provides cloneSchema and cloneType methods that take the place of createResolveType.

fieldMaptoFieldMapConfig is still there, but I don't think it is actually exported.

New to development with Gatsby, however, could you point me perhaps to the right set of integration tests to run when touching this up?

@yaacovCR
Copy link
Contributor Author

Fixed by #20042.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants