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.

Sign up for GitHub

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

Jump to bottom

File upload concept - Taken from #116 #626

Merged
merged 7 commits into from
Jul 18, 2019
Merged

File upload concept - Taken from #116 #626

merged 7 commits into from
Jul 18, 2019

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Jul 11, 2019

This code is taken from #116

@kimdv
Copy link
Contributor Author

kimdv commented Jul 13, 2019

@designatednerd I have started the PR.
Tests are passing.

Now I need to figure out how to test the upload part.
Can you guide me here?

@designatednerd designatednerd mentioned this pull request Jul 15, 2019
Closed
designatednerd
designatednerd reviewed Jul 15, 2019
View reviewed changes
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few preliminary comments.

For testing, something that would be really helpful is to make sure that output from MultipartFormData matches what's expected in the GraphQL File Upload Spec.

Sources/Apollo/GraphQLFile.swift Show resolved Hide resolved
Sources/Apollo/HTTPNetworkTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/HTTPNetworkTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/MultipartFormData.swift Outdated Show resolved Hide resolved
Sources/Apollo/NetworkTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/GraphQLFile.swift Outdated Show resolved Hide resolved
@kimdv
Copy link
Contributor Author

kimdv commented Jul 15, 2019

@designatednerd I have resolved the comments in the code!
I reached out on Spectrum with some questions 🚀

@designatednerd designatednerd mentioned this pull request Jul 15, 2019
Closed
designatednerd
designatednerd reviewed Jul 15, 2019
View reviewed changes
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looking forward to the tests!

Sources/Apollo/HTTPNetworkTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/MultipartFormData.swift Outdated Show resolved Hide resolved
@kimdv kimdv marked this pull request as ready for review July 16, 2019 12:26
@kimdv
Copy link
Contributor Author

kimdv commented Jul 16, 2019

@designatednerd rebased and made it ready for review 🚀

@designatednerd designatednerd mentioned this pull request Jul 16, 2019
Closed
designatednerd
designatednerd approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any comments I have are anal-retentive formatting nonsense I can fix myself after merging. Thank you so much for this! Gonna leave it open until tomorrow at 12:00 CEST (11:00 GMT) for any further comments or objections, and then I'll merge!

@kimdv
Copy link
Contributor Author

kimdv commented Jul 17, 2019

Thanks a lot! 🚀

@designatednerd
Copy link
Contributor

Time's up, let's ship this!

@designatednerd designatednerd merged commit 1ff23e3 into apollographql:master Jul 18, 2019
@designatednerd designatednerd added this to the 0.13.0 milestone Jul 18, 2019
@kimdv kimdv deleted the kimdv/graphql-file-upload branch July 18, 2019 11:46
@designatednerd designatednerd mentioned this pull request Jul 18, 2019
Merged
@Drusy
Copy link

Drusy commented Jul 30, 2019

Hello @designatednerd, could you provide a concret example how to use file upload ?
I can't find any official documentation on this topic.
How the .graphql file should look ?
Where GraphQLFile instances should be passed ?

Thanks in advance

@designatednerd
Copy link
Contributor

@Drusy Mind opening the lack of documentation as a new issue? I'll try to get that going later this week. The short answer is that you should use the upload function in the HTTPNetworkTransport.

@Drusy
Copy link

Drusy commented Jul 30, 2019

Linked to #681 (File upload v0.13 - Missing documentation )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@designatednerd designatednerd designatednerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kimdv @designatednerd @Drusy