Skip to content
This repository has been archived by the owner on Aug 20, 2020. It is now read-only.

Enable file uploads via mutation or query input variables. #8

Closed
wants to merge 3 commits into from

Conversation

jaydenseric
Copy link
Contributor

@jaydenseric jaydenseric commented Jul 13, 2017

Files can now be uploaded via mutation or query input variables, implementing the apollo-upload-client multipart request method. Users must setup apollo-upload-server to use this feature until support is added to graphql-server. An unmodified API server will continue to work fine if users don't attempt uploads.

You can try it out by cloning the apollo-upload-examples fetch branch. Use npm link to try it out with this PR.

Fixes #6.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@jaydenseric
Copy link
Contributor Author

I dropped setting the Accept: '*/*' header in the fetch options as I don't think it is necessary and I have not been using it in apollo-upload-client for a long time now. Let me know if there is something about that I am missing.

Note that according to the fetch spec, setting the Content-Type header is unnecessary when the body is of type FormData.

fetchOptions.body = new FormData();
fetchOptions.body.append('operations', JSON.stringify(request));
files.forEach(({ path, file }) => fetchOptions.body.append(path, file));
} else {
Copy link

Choose a reason for hiding this comment

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

Missing fetchOptions.headers['Content-Type'] = 'multipart/form-data';?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, as per the fetch spec it gets set automatically when the body is FormData.

@jaydenseric
Copy link
Contributor Author

@stubailo @evanshauser @jbaxleyiii very excited to hear your thoughts and see this progress.

It's taken the week to get this PR together so I have to get back onto project work now. It would be great if whoever is most familiar with the tests here could update them for this functionality. If everyone is happy with the approach I can help out updating the docs, although you should be able to pinch paragraphs from apollo-upload-client.

Once graphql-server is updated the DX will be super intuitive; zero config uploads.

@vladshcherbin
Copy link

@jaydenseric will it be possible to get the upload progress so progress bar (or percentage) can be shown on client?

@jaydenseric
Copy link
Contributor Author

@vladshcherbin unfortunately no, as fetch doesn't have a way to track upload progress. If we switched to old-school XHR it might be possible. Apollo would need a new progress API which would be handy for all requests, not just uploads.

Keep in mind that the actual upload is sometimes just part of the wait for a mutation response – the server takes time to process the request and that is not easy to assign progress. Then the response download happens. So the UI might be loading progress, then a spinner, then perhaps a brief progress again.

@vladshcherbin
Copy link

@jaydenseric maybe it's a good idea to consider XHR from the beginning because while for a regular form the loading percentage is not that relevant, for a file upload it really is.

If implemented, it gives better UX and is really useful when you upload multiple files and want to show percentage for each of them or your mutation just uploads file on dropzone file drop.

@jaydenseric
Copy link
Contributor Author

@vladshcherbin this PR (and apollo-upload-client) allows files to appear to be a part of the query itself, like any other query variable. My vision is to provide a GraphQL solution as intuitive and generic as possible, rather than create an opinionated framework.

A new Apollo API for tracking query progress would be handy and could be implemented later as an enhancement to benefit all large queries; not just ones containing files. This wouldn't show upload progress for individual files – it would be for the GraphQL request as a whole including all files within variables and query JSON. If you need to track uploads separately you could make seperate mutation requests.

This hypothetical progress API has been suggested before and might never happen. Having uploads without progress is still very useful to a lot of people now, evidenced by the exponential growth of apollo-upload-client installs.

This PR will not be the only way to upload files in an app, people can still use XHR and a traditional REST endpoint.

@vladshcherbin
Copy link

@jaydenseric while the whole apollo query tracking may be useful, I don't understand why we need to wait for it and for file upload progress when we can have it now while adding file upload functionality.

People will be still using apollo-upload-client package as it seems like the only good solution for apollo now, I will also use it and miss the progress functionality. Using rest or xhr for me is strange when I have graphql for everything.

Anyway, I respect all you work and decisions, this was just the feature I'd love to have. Thank you!

@jaydenseric
Copy link
Contributor Author

I would like the feature too. It will be an effort across several repos.

The network interfaces would need to be re-written to XHR, then we need to establish a way for them to report query progress to Apollo Client. Batching could complicate things as progress would be shared. React Apollo would then need an API for progress callbacks and props when decorating components. Changes would need to be made to each moving part with compatibility in mind so nothing breaks.

@evans
Copy link
Contributor

evans commented Jul 21, 2017

@jaydenseric Thank you so much for contributing and adding to the community! Very sorry for the lack of response. We were pushing hard for a prerelease of apollo-link

We've been discussing apollo-fetch and want to make the repository more modular with apollo-fetch-upload as a separate package. I am going to make a PR in a couple of hours with lerna support and an additional parameter in createApolloFetch for constructOptions(GraphQLRequest, RequestInit) => RequestInit that allows the creation of the options passed to fetch(the parsing done in callFetch)

The reason to make the constructOptions a parameter is so consumers can add their own formats for the options/body if necessary. apollo-fetch will export a default construct, so that other fetchs, such as your apollo-fetch-upload can check for file upload and otherwise call the default.

The PR will also include batching support, so the signature of constructOptions will probably end up as (GraphQLRequest | GraphQLRequest[], RequestInit) => RequestInit. I'll ping you when the PR is open.

If you have any early concerns, please let me know. Thank you again for taking charge of file upload in apollo-client!

@evans
Copy link
Contributor

evans commented Jul 26, 2017

@jaydenseric #10 is merged, so batching and constrctOptions are now available in 0.4.0. Can't wait to see the apollo-fetch-upload package in the repo! Please let me know if you need any more support.

@jaydenseric
Copy link
Contributor Author

@evanshauser thanks, working on it now. I might Slack you about using Lerna to locally dev and test a new sub package.

@jaydenseric
Copy link
Contributor Author

Closing in favor of #14.

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

Successfully merging this pull request may close these issues.

None yet

4 participants