-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(gatsby-source-graphql): use embedded remote schemas #20042
Conversation
avoids additional layer of delegation
avoids additional layer of delegation
…CR/gatsby into topics/graphql-tools-fork
Sorry about my initial confusion as to which tests to run! |
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. |
Please let me know if there are additional testing steps to run through beyond |
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. |
Looks to me like unrelated test failing? Anything else I can do to help push this forward? |
@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. :) |
Maybe some kind users of gatsby-source-graphql could test version in this PR and confirm working? @heldrida @acatxnamedvirtue @truecolintracy @emaddoma @herrwitzi @JacksonMalloy @ikisoft |
@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. |
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!!! |
@heldrida any luck? Do those instructions work for you? |
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. |
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 %) |
:) just grateful you guys haven't given up on me. Sorry about the prior trouble. |
There was a problem hiding this 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
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.