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(gatsby-source-graphql): use embedded remote schemas #20042

Merged
merged 7 commits into from
Dec 19, 2019

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Dec 10, 2019

Second attempt at: #19504, no longer relying on non-exported internal methods in graphql-tools or graphql-tools-fork.

Fixes:
#20015

Updated the example package, verified tests failing with initial PR, fixed now.

@yaacovCR yaacovCR requested a review from a team as a code owner December 10, 2019 15:42
@yaacovCR
Copy link
Contributor Author

@freiksenet @stefanprobst

Sorry about my initial confusion as to which tests to run!

@yaacovCR yaacovCR changed the title Topics/graphql tools fork graphql-tools-fork and embedded remote schemas Dec 10, 2019
@yaacovCR yaacovCR changed the title graphql-tools-fork and embedded remote schemas feat(gatsby-source-graphql): use embedded remote schemas Dec 10, 2019
@yaacovCR yaacovCR changed the title feat(gatsby-source-graphql): use embedded remote schemas fix(gatsby-source-graphql): use embedded remote schemas Dec 11, 2019
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Dec 11, 2019

NamespaceUnderFieldTransform now basically works just like the WrapType transform exported and tested within graphql-tools-fork, except it allows you to specify the field resolver rather than just passing down the initial rootValue. Eventually I should allow that option within WrapType and just use that.

healSchema is part of the graphql-tools framework for schema directives, which together with visitSchema has been integrated within the fork into the schema transformation framework. You can now modify the schema in place and then heal it to make sure that all types/fields are rewired correctly.

This replaces the createResolveType method and in this case makes fieldMapToFieldConfigMap unnecessary.

@yaacovCR
Copy link
Contributor Author

Please let me know if there are additional testing steps to run through beyond
https://github.com/gatsbyjs/gatsby/tree/master/examples/using-gatsby-source-graphql.

@yaacovCR
Copy link
Contributor Author

Pinging @freiksenet @vladar @stefanprobst

Hopeful that window for this has not closed... :-/

Development on upstream graphql-tools has essentially stopped and I am hopeful to get graphql-tools-fork out to the larger community so that work on remote schema proxying can continue...

Would love to discuss further offline.

@yaacovCR
Copy link
Contributor Author

Looks to me like unrelated test failing? Anything else I can do to help push this forward?

@yaacovCR
Copy link
Contributor Author

@vladar @freiksenet @stefanprobst

Anything else you might want to see in terms of tests?

Am hopeful you will still give this a chance.

Let me know if I should give it a rest. :)

@yaacovCR
Copy link
Contributor Author

Maybe some kind users of gatsby-source-graphql could test version in this PR and confirm working?

@heldrida @acatxnamedvirtue @truecolintracy @emaddoma @herrwitzi @JacksonMalloy @ikisoft

@heldrida
Copy link

@yaacovCR I haven't personally experienced working with the source plugins directly, if you drop instructions about how to have the PR version in my dev environment (even if that just means pull to a node_modules directory), once I get a few minutes I'll have a look.

@yaacovCR
Copy link
Contributor Author

Thanks! I tested on my cloned repo by using https://www.npmjs.com/package/gatsby-dev-cli from the examples folder using-gatsby-source-graphql

I followed instructions here to set up my local environment: https://www.gatsbyjs.org/contributing/setting-up-your-local-dev-environment/

I then ran gatsby-dev from the example repo

Really appreciate your time!!!

@yaacovCR
Copy link
Contributor Author

@yaacovCR
Copy link
Contributor Author

@heldrida any luck? Do those instructions work for you?

@yaacovCR yaacovCR added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response needs manual testing topic: GraphQL Related to Gatsby's GraphQL layer labels Dec 13, 2019
@yaacovCR
Copy link
Contributor Author

Main advantage for this PR is correct forwarding of errors with extensions from original schema. I certainly can understand hesitation given problem with first attempt, but pinging @freiksenet @stefanprobst @vladar anyway because I am still hopeful. :)

Please feel free to close PR if the window for switching has essentially closed. I think failure of #19504 was essentially do to my oversight of the appropriate tests to run. I have added several new tests and verified things look good in example repository. Would love if anyone could reach out about any additional test suggestions.

Thanks, appreciate you taking the time.

@vladar
Copy link
Contributor

vladar commented Dec 16, 2019

We just need to perform another pass of manual testing and so need to dedicate some time to this. Will probably do it tomorrow. Then we can continue. Sorry if it takes a while %)

@yaacovCR
Copy link
Contributor Author

:) just grateful you guys haven't given up on me. Sorry about the prior trouble.

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

I tried it locally and it seems to be working (even with schema rebuilding). But I don't feel my expertise in schema stitching is enough to merge. Prefer to wait for another pair of eyes. CC @freiksenet @stefanprobst

@freiksenet freiksenet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 19, 2019
@gatsbybot gatsbybot merged commit 5bc8b79 into gatsbyjs:master Dec 19, 2019
@yaacovCR yaacovCR deleted the topics/graphql-tools-fork branch December 25, 2019 01:46
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 status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants