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

Add an apollo-fetch-upload package #14

Merged
merged 17 commits into from
Aug 10, 2017

Conversation

jaydenseric
Copy link
Contributor

@jaydenseric jaydenseric commented Jul 26, 2017

An apollo-fetch-upload package to fix #6, superseding #8.

This package allows the use of files anywhere in a GraphQL request (in theory, not just in variables) and is compatible with apollo-upload-server.

To try it out:

  1. Clone this PR.
  2. Within apollo-fetch/packages/apollo-upload-fetch run npm install && npm link.
  3. Clone the apollo-upload-examples apollo-fetch-upload branch.
  4. Within apollo-upload-examples/app run npm install && npm link apollo-upload-fetch. You may have to manually remove apollo-upload-fetch from the dependencies first since it is not published yet.
  5. Following the remain setup instructions for apollo-upload-examples.

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

- Nicer package.json config ordering.
- Cleaner dist directory structure (no redundant src subdirectories).
- Added an ES6 module entry to the package for modern bundlers like Rollup and Webpack.
- Removed redundant glob argument from the lint script.
@jaydenseric jaydenseric changed the title Apollo fetch upload Add an apollo-fetch-upload package Jul 26, 2017
@jaydenseric
Copy link
Contributor Author

I haven't testing it with batching yet, but it should work.

@jaydenseric
Copy link
Contributor Author

There is a FetchOptions type I would like to use, but #13 needs to be merged first so it can be imported from apollo-fetch.

requestOrRequests: GraphQLRequest | GraphQLRequest[],
options: RequestInit,
): RequestInit {
const files = extractFiles(requestOrRequests);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evanshauser Does it matter that requestOrRequests is modified by extractFiles? Or would it be better if extract files did not modify the input:

const { files, modifiedTree } = extractFiles(requestOrRequests)

Keep in mind that there are performance considerations to messing around with objects that contain potentially megabytes of files.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaydenseric currently, reqeustOrRequests isn't used after it is incorporated into the fetch options, so should be safe to modify in constructOptions/we don't need a defensive copy in apollo-fetch.

In general from apollo-fetch's perspective, once we have called an external function, such as constructOptions, the references passed as arguments should not be used by apollo-fetch

@jaydenseric
Copy link
Contributor Author

jaydenseric commented Jul 27, 2017

I think the implementation is ready.

@evanshauser are you able to explain how to get batching to work here so we can test it out?

Help setting up the tests will be much appreciated, I'm not really sure how to go about it. extract-files has it's own tests, so we just need to test the APIs defined in this package.

@evans
Copy link
Contributor

evans commented Jul 27, 2017

@jaydenseric awesome work!

The way to get batching enabled is (batch blog):

const client = new ApolloClient({
 // ... other options ...
 shouldBatch: true,
});

This option would mean that all queries would be put into a single batch, so file uploads and regular GraphQL requests would be placed in the same network request. Does your apollo-fetch-upload differentiate/handle the two during constructOptions?

Also, can FormData encode regular queries? If it's not compatible with the default stringify request, then one idea is to a function for apollo-link's split that routes file uploads to one Link and the rest to another. Something like:

screen shot 2017-07-27 at 2 55 01 pm

We're working on a BatchingLink that will perform the batching logic and run a provided function on the batch.

@jaydenseric
Copy link
Contributor Author

@evanshauser I'm quite familiar with batching, and shouldBatch is deprecated. The current way is to use batchingNetworkInterface:

import ApolloClient, { createBatchingNetworkInterface } from 'apollo-client'

const client = new ApolloClient({
  networkInterface: createBatchingNetworkInterface({
    uri: '/graphql'
  })
})

But with apollo-fetch, networkInterface is replaced with:

networkInterface: { query: req => apolloFetchUpload({ ...req, query: print(req.query) }) }

apollo-fetch-upload works the same with regular or batched requests. If a request or batch of requests contains no files at all, the request is sent as default, without FormData.

@stubailo
Copy link
Contributor

Yeah - right now all of the logic for batching (collecting multiple requests and sending them as one) lives inside the network interface implementation. I think the correct solution here is to use apollo-link to do the batching, which will pass down an array of requests down to apollo-fetch. I think @evanshauser is working on a batching link as we speak.

@evans
Copy link
Contributor

evans commented Aug 1, 2017

@jaydenseric This looks great! I think we are ready to merge after unit tests for getting the transformation tested for a single request(upload and normal with/without an array) and batched request(two normal, two uploads, and one of each). Not sure about the different errors that can occur from a failed upload, so I'll trust you to include what you feel is relevant. Then after the changelogs are per project, we can announce apollo-fetch-upload!

So excited, this will be an awesome addition!

We're also planning to add some Continuous deployment into Travis-CI, so if you could add the npm user, mdg, to the apollo-fetch-upload, we can start to hook things up.

@jaydenseric
Copy link
Contributor Author

@evanshauser So, it's been an unproductive day attempting to create tests. Here are some of the challenges...

File and FormData APIs are not available in Node.js. It seems that there is very little info on unit testing browser code that uses these APIs in Node.js. It's an unsolved problem?

I attempted to polyfill FormData using packages such as form-data but appending files is different to web standards; they don't understand File instances. I could not find a suitable polyfill for File and even if I did, the FormData polyfill would not play nice.

For these reasons there are no tests in apollo-upload-client. Maybe we should publish a v1 without all the tests we would like to have. For now apollo-fetch-upload is opt-in so I don't think its unreasonable. extract-files has tests, and that's where the most confusing bugs originate.

@HriBB
Copy link

HriBB commented Aug 9, 2017

How do supertest and superagent do it?

In this case I am starting a Koa server, then sending a multipart/form-data request to the /graphql endpoint. Maybe you could pass this data directly to the test?

import Koa from 'koa'
import body from 'koa-bodyparser'
import { resolve } from 'path'
import request from 'supertest'
import { expect } from 'chai'
import 'mocha'

const app = new Koa()

app.use(body())

createGraphQLServer(app)

const server = app.listen(4004)

describe('GraphQL Server', () => {

  it('can upload images', () => {
    return request(server)
      .post('/graphql')
      .field('operationName', 'uploadImages')
      .field('query', `mutation uploadImages($location_id: Int!, $images: [File!]!) {
        uploadImages(location_id: $location_id, images: $images) {
          name slug width height file_name extension mime_type url
          owner { id username }
        }
      }`)
      .field('variables', JSON.stringify({
        location_id: 3,
      }))
      .attach('images', resolve(__dirname, 'images', 'image1.jpg'))
      .attach('images', resolve(__dirname, 'images', 'image2.jpg'))
      .attach('images', resolve(__dirname, 'images', 'image3.jpg'))
      .expect(200)
      .then(res => {
        expect(res.body).to.deep.equal(expected)
      })
  })

})

Probably I'm missing something ...

@evans evans merged commit 7325f50 into apollographql:master Aug 10, 2017
@evans
Copy link
Contributor

evans commented Aug 10, 2017

@jaydenseric This is incredible work! Great job! So happy to have apollo-fetch-upload

@jaydenseric jaydenseric deleted the apollo-fetch-upload branch August 11, 2017 16:11
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.

Feature: File upload?
4 participants