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

Add file uploads [RFC] [WIP] #428

Closed
wants to merge 11 commits into from

Conversation

JustinDSN
Copy link
Contributor

@JustinDSN JustinDSN commented Jan 8, 2019

Add Upload Support to Apollo-iOS

Depends on:

Summary of Changes:

  • Adds a new protocol GraphQLUpload and typealias for Upload.
  • Adds new structs for DataGraphQLUpload, FileGraphQLUpload, InputStreamGraphQLUpload to allow clients flexibility to supply an uploads.
  • Adds a new protocol GraphQLUploadOperation which inherits from GraphQLOperation.
  • Adds a new protocol GraphQLUploadMutation which inherits from GraphQLUploadOperation.
  • Adds a public function to ApolloClient to perform GraphQLUploadMutations
  • Changes generic constraints on Operation type on NetworkTransport protocol.
  • Adds a new requirement to send<Operation: GraphQLUploadOperation> on NetworkTransport protocol.
  • Added implementation for send<Operation: GraphQLUploadOperation> in HTTPNetworkTransport.
  • Adds AlamoFire's MultipartForm, AFError, and HTTPHeaders to provide robust MultipartFrom encoding.

TODO:

  • [] Change apollo-cli to generate GraphQLUploadMutations correctly.
  • [] Implement multipart encoding memory thresholds to either write to disk or stream from memory.

@JustinDSN JustinDSN changed the title Add file uploads [RFC] [WIP} Add file uploads [RFC] [WIP] Jan 8, 2019
import Foundation

public protocol GraphQLUpload: JSONEncodable {
var data: Data { get }
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

@MrAlek MrAlek left a 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
Copy link
Contributor

@martijnwalraven martijnwalraven left a 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?

Sources/Apollo/AFError.swift Outdated Show resolved Hide resolved
Sources/Apollo/ApolloClient.swift Outdated Show resolved Hide resolved
Sources/Apollo/ApolloClient.swift Outdated Show resolved Hide resolved
Sources/Apollo/GraphQLOperation.swift Outdated Show resolved Hide resolved
@victormihaita
Copy link

Hi guys. Do you have by any chance any updates for this PR?

JustinDSN added 2 commits May 10, 2019 21:16
…into feature/file-uploads

# Conflicts:
#	Apollo.xcodeproj/project.pbxproj
- Also renamed the error type to address PR feedback
@JustinDSN
Copy link
Contributor Author

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:

  • [] Review and merge Bump js-yaml from 3.8.3 to 3.13.1 starwars-server#10, this will allow the unit-tests I've written in this PR to pass.
  • [] Change apollo-cli to generate GraphQLUploadMutations correctly as a separate PR. Can someone from the Apollo team help with this?
  • [] Implement multipart encoding memory thresholds to either write to disk or stream from memory as a separate PR.

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?

@victormihaita
Copy link

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

@designatednerd
Copy link
Contributor

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?

@designatednerd
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants