-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Schema stitching support #172
Comments
I'm sorry, I'm not across all the moving parts and I'm a bit rusty since I looked into it over a year and a half ago. To get I suspect though that schema stitching is not a valid use case for this. I wouldn't use Apollo Client on the server, I would just use |
I think you are right that I can just tap into extractFiles options. Seeing as they really should not be any promises in the variables map, I can just await all promises first and then replace them with the resolved values and then pass to extractFiles. Schema stitching does not use a cache, it uses a bare Apollo link that acts like a fetch-like fetcher unless using subscriptions. |
Multiple kilobytes of needless complexity and dependencies? Just make a I may be missing something, you've spent more time thinking about it than I have :) The only thing actionable here I think is for someone to PR a new |
An aside: you can use a fetcher instead of a link in schema stitching if you want. I guess my issue is that in graphql-tools and the fork, we try to make schema stitching straightforward and provide default delegating resolvers. The question I have is where to resolve the promises in the resolver arguments, I had thought that could be integrated into extractFiles as well but I see now that it maybe too much work to turn that into an asynchronous function. I wonder if graphql-tools-fork should be responsible for resolving the promises within the variables map and the conversion of the resolved file object into the same format as expected in the upload client (essentially passing the result of createReadStream with name property set to filename) and then extractFiles can handle the further extraction of variables into the map. |
Related: node-fetch/node-fetch#707 |
graphql-tools-fork v8.1.0 provides new exports to allow proxying remote file uploads for those who want to proxy all the things. The new createServerHttpLink method should be used to set up a terminating link to a subschema; it will resolve all promises within the variables, and then, if appropriate, append the File or stream to the FormData. It includes an extended version of the FormData class that works even with the old version of node-fetch. I based the createServerHttpLink on the latest version of apollo-link-http and apollo-client-upload. Would be happy to submit it as a PR, but it is intended for use on the server rather than the client. Perhaps it belongs in a separate project? The GraphQLUpload scalar export from graphql-tools-fork just de-disables the serialize method as all args as serialized prior to proxying to the subschema. Feedback/critiques/suggestions would be much appreciated. I am happy to submit PRs where appropriate, this is more POC. |
@yaacovCR Does proxying remote file uploads allow for stitching of remote schemas, in which one of those subschemas passes a File upload as a multipart/form content-type? I was taking a look at the source code in graphql-tools-fork, but couldn't get this working. If you could send me some example documentation, I'll be happy to give feedback after implementing and contribute to any related issues. Thanks! |
Yes, that is exactly what it is supposed to support. See test for now for example: https://github.com/yaacovCR/graphql-tools-fork/blob/master/src/test/testUpload.ts Unfortunately, the new features in graphql-tools-fork suffer from a paucity of documentation, see yaacovCR/graphql-tools-fork#21 I am looking for help with the repo in general as Apollo has essentially stopped supporting graphql-tools, docs would be a great place. |
Just noticed that the test defines an upload resolver in mergeSchemas, that resolver should at this point essentially be the default stitching resolver and not necessary, just had it in there when debugging, will update when I get a chance, see https://github.com/yaacovCR/graphql-tools-fork/blob/master/src/test/testUpload.ts#L110 You should be able to just take that out, simplifying the gateway further. |
@yazfaz if you have a code example of the new functionality not working, I would love to take a look |
@yaacovCR Thanks, that would be very helpful! I'll update my code with the new info you provided and open an issue on the graphql-tools-fork repo with the code. I'll definitely submit a PR for documentation on this once it's working. |
See https://github.com/yaacovCR/graphql-tools-fork/blob/master/src/test/testUpload.ts Gateway code has been streamlined to just the basics even when proxying file uploads:
Magic happens within the
You can also still define the subschema passed to
|
@jaydenseric Any thoughts as to whether you would want a PR? Basically, apollo-upload-client is described as:
A PR would change that to:
I think with this more generic support, a better name might be |
@yazfaz have you had a chance to look into this? |
@yaacovCR Sorry for the delay- I've been off this project for the last few weeks, but will be implementing the graphql-tools-fork code this week. I'll be sure to update you and submit a documentation PR then, thanks! |
@yazfaz when this fork will be merged , i'm trying to using gateway with file upload . |
@jaydenseric I think #179 is nice and generic, the fact that it happens to support schema stitching is sort of a bonus, main idea is that it passes isExtractableFile to extractFiles, and allows customization of the FormData polyfill analogous to customization of fetch. Hopefully you will have a chance to review. |
Support uploading files from a server environment. Fixes #172 .
Support for uploading files from a server environment has been published in v13.0.0 🚀 |
Hi!
I have been working on graphql-tools-fork and although I am not sure myself that gateway forwarding of files to subschemas is the best idea, it certainly has been a requested idea (jaydenseric/graphql-upload#56, ardatan/graphql-tools#671), and I am hoping to support it within the fork.
One approach based on your comment (#79 (comment)) is to expand extract-files to just handle streams in addition to files.
This is a bit tricky for my use case, as what the fork does automatically is populate the delegated variables with the value of the variable from the first schema, i.e. a promise that resolves to an object with a createReadStream method, not a stream itself.
My thought is to spend a bit longer parsing the query based on the schema and to actually look for variables of type GraphQLUpload. Then I will await know that those promises if they successfully resolve will resolve appropriately to the correct streams, and I will await Promise.all() them. I think I can handle batching there, but I am not quite sure.
What do you think about this approach? It would require passing the schema manually on link creation, I believe, but otherwise I think it would be doable?
Do you think it belongs in a PR to this repository or a separate project?
The text was updated successfully, but these errors were encountered: