Add file uploads [RFC] [WIP]#428
Conversation
| import Foundation | ||
|
|
||
| public protocol GraphQLUpload: JSONEncodable { | ||
| var data: Data { get } |
There was a problem hiding this comment.
This property should preferably be a stream instead of actual bytes. For large upload, you don't want to load the entire file into memory at once.
You mentioned you have had a look at https://github.com/Alamofire/Alamofire/blob/master/Source/MultipartFormData.swift ?
There was a problem hiding this comment.
I just pushed a revision that supports Data, File, and InputStream variants of uploads.
MrAlek
left a comment
There was a problem hiding this comment.
Great initiative! At Dooer, we've implemented uploads very similarly with the exception of using Alamofire's multipart serializer, utilizing streams.
- This allows separate code-paths for operations with and without uploads.
- Copy and pasted from Alamofire’s Repo (leaving attribution comments) - Added DataGraphQLUpload, FileGraphQLUpload, InputStreamGraphQLUpload
martijnwalraven
left a comment
There was a problem hiding this comment.
This looks great!
I hope we can avoid the code duplication for GraphQLUploadMutation however, and rely on protocol composition instead. There might be subtleties here in how method dispatch works, so we may need more explicit checks to dispatch to the right method in HTTPNetworkTransport, but the idea would be to use GraphQLMutation & GraphQLUploadOperation as the type instead of GraphQLUploadMutation.
Does that make sense to you or am I missing something?
|
Hi guys. Do you have by any chance any updates for this PR? |
…into feature/file-uploads # Conflicts: # Apollo.xcodeproj/project.pbxproj
- Also renamed the error type to address PR feedback
|
Hi @victormihaita, I'm sorry for the delay, but this PR took a back burner for a little while. I've addressed the comments left by @martijnwalraven, but it's been a while since we've collaborated on this, so we'll have to see if we can get this off the shelves. @martijnwalraven from my perspective heres the checklist:
What do you think? Do you think we should approach it this way or some other way. I'm also very curious how many members of the community are looking for this? |
|
This feature is something that we are really looking forward to have it. Right now, we had to create a separate REST Endpoint in our API just for uploading images (while the rest of it is based on GraphQL), and this was made just for the iOS client... I consider it as a really important feature for a networking service such as Apollo... it would be great if we can see any progress in this direction :) @martijnwalraven @JustinDSN |
|
Hey @JustinDSN - thanks for your hard work on this, and apologies this was left fallow for so long. Do you have any thoughts on rebasing this vs moving forward with the simpler implementation in #626? |
|
I've gone ahead and merged #626 - I'm going to close this one out, but please feel free to open a new PR if there's anything that's missing from the implementation there! |
Add Upload Support to Apollo-iOS
Depends on:
Summary of Changes:
GraphQLUploadandtypealiasforUpload.structs forDataGraphQLUpload,FileGraphQLUpload,InputStreamGraphQLUploadto allow clients flexibility to supply an uploads.GraphQLUploadOperationwhich inherits fromGraphQLOperation.GraphQLUploadMutationwhich inherits fromGraphQLUploadOperation.ApolloClienttoperformGraphQLUploadMutationsOperationtype onNetworkTransportprotocol.send<Operation: GraphQLUploadOperation>onNetworkTransportprotocol.send<Operation: GraphQLUploadOperation>inHTTPNetworkTransport.MultipartForm,AFError, andHTTPHeadersto provide robust MultipartFrom encoding.TODO:
GraphQLUploadMutationscorrectly.