-
Notifications
You must be signed in to change notification settings - Fork 738
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
Add file uploads [RFC] [WIP] #428
Conversation
Sources/Apollo/GraphQLUpload.swift
Outdated
import Foundation | ||
|
||
public protocol GraphQLUpload: JSONEncodable { | ||
var data: Data { get } |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a revision that supports Data
, File
, and InputStream
variants of uploads.
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.
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
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.
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:
GraphQLUpload
andtypealias
forUpload
.struct
s forDataGraphQLUpload
,FileGraphQLUpload
,InputStreamGraphQLUpload
to allow clients flexibility to supply an uploads.GraphQLUploadOperation
which inherits fromGraphQLOperation
.GraphQLUploadMutation
which inherits fromGraphQLUploadOperation
.ApolloClient
toperform
GraphQLUploadMutation
sOperation
type onNetworkTransport
protocol.send<Operation: GraphQLUploadOperation>
onNetworkTransport
protocol.send<Operation: GraphQLUploadOperation>
inHTTPNetworkTransport
.MultipartForm
,AFError
, andHTTPHeaders
to provide robust MultipartFrom encoding.TODO:
GraphQLUploadMutations
correctly.