Add lerna support and batched requests #10

evans · 2017-07-22T00:13:26Z

apollo-fetch is now in the lerna ecosystem and supports batching, while still being compatible with the previous single request API.

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

evans · 2017-07-24T06:09:42Z

@jaydenseric if you get a chance, could you look a this PR? I want to provide you the ability to create an upload client and include it as a lerna package, so that your upload client can take easy advantage of integration test. Do you see a way forward if we pull this in?

I envision that your upload fetch package could export either a createApolloFetchUpload, an ApolloFetch with upload capabilities, or a createUploadOptions function. Something that might look like this:

import {
  createApolloFetch,
  createDefaultOptions,
}

export function constructUploadOptions(requestOrRequests, options) {
  if( isFileUpload(requestOrRequests, options)) {
    return {
      ...options,
      body: extract-files(requestOrRequests),
    }
  } else {
    return constructDefaultOptions(requestOrRequests, options);
  }
}

export createApolloFetchUpload(params) {
  return createApolloFetch({
    ...params,
    constructOptions: createUploadOptions,
  })
}

const apolloFetchUpload = createApolloFetchUpload();
export apolloFetchUpload;

Please let me know if you need more hooks in ApolloFetch or anything in the PR is unclear.

In the whirlwind, I never got to mention that your extract-files package looks great!

jaydenseric · 2017-07-24T06:29:08Z

Thanks 😊

This approach should work; I'll try to have the packages ready in the next few days. Working on them will be a bit easier after this PR is merged.

stubailo

Looks legit! A few requests for improvements.

stubailo · 2017-07-24T07:04:57Z

README.md

@@ -1,9 +1,12 @@
 # apollo-fetch [![npm version](https://badge.fury.io/js/apollo-fetch.svg)](https://badge.fury.io/js/apollo-fetch) [![Get on Slack](https://img.shields.io/badge/slack-join-orange.svg)](http://www.apollostack.com/#slack)


-`apollo-fetch` is a lightweight client for GraphQL requests that supports middleware and afterware that modify requests and responses.
+`apollo-fetch` is a lightweight client for single and batched GraphQL requests that supports middleware and afterware that modify requests and responses.


I think this dilutes the description. Batching is a feature, no need to mention it in the tagline.

stubailo · 2017-07-24T07:05:46Z

README.md

+These transformations can be useful for servers that have different formatting for batches or other extra capabilities.
+
+```js
+const constructOptions = (requestOrRequests, init) => {


I'd add a comment here about what requestOrRequests means.

stubailo · 2017-07-24T07:06:20Z

README.md

  use: (middlewares: MiddlewareInterface) => ApolloFetch;
  useAfter: (afterwares: AfterwareInterface) => ApolloFetch;
+  batchUse: (middlewares: BatchMiddlewareInterface) => ApolloFetch;


Is BatchMiddlewareInterface defined anywhere?

stubailo · 2017-07-24T07:12:25Z

docs/batch.md

+
+### Batch Formatting
+
+Since GraphQL does not include batched requests in the formal spec, `apollo-fetch` creates a request that is compatible with [`apollo-server`](https://github.com/apollographql/apollo-server).


GraphQL does not include batched requests in the formal spec

No kind of request is mentioned in the spec. So no need to make this exception here.

Maybe say "not all GraphQL servers support batching out of the box" or something.

stubailo · 2017-07-24T07:13:25Z

docs/monorepo.md

+Copying the `apollo-fetch` folder will provide a good starting point for revision and additions.
+Including new fetch's in the lerna project, will allow packages have integration tests with any core changes.
+
+The typescript specific functionality is encapsulated within a package, so ou are also able to create a fetch with javascript to include here.


Hmmm.. I think we'd really like to use the same language for everything in the same repo.

stubailo · 2017-07-24T07:13:50Z

docs/monorepo.md

+
+## Creating a new fetch
+
+If you are creating a new fetch and would like to included in the lerna repository, the easiest place to start is copying the `apollo-fetch` folder in `<repository root>/packages`.


Why would people want it in this repository? For testing reasons? I feel like this should include a note about what should actually be in here.

@stubailo I think this is intended for other contributors to PR and add in new packages if the need arises

jbaxleyiii

@evanshauser a couple questions but otherwise this looks great!

jbaxleyiii · 2017-07-24T13:04:22Z

README.md


 By default `apollo-fetch` uses `isomorphic-fetch`, but you have the option of using a custom fetch function.
+Additionally, `apollo-fetch` allows specification of how GraphQL requests are included in fetch options.


This sentence is awkward? I'm not sure what it is explaining

jbaxleyiii · 2017-07-24T13:06:43Z

README.md

+const apolloFetch = createApolloFetch({ constructOptions });
+
+//simplified usage inside apolloFetch
+fetch(uri, constructOptions(request, options)) //options are result of middleware


For this section, I think it could be helpful to include (not inline) the type definitions of request and options so developers can know what will be present when creating their own functions

jbaxleyiii · 2017-07-24T13:07:11Z

README.md

@@ -261,21 +283,27 @@ createApolloFetch(options: FetchOptions): ApolloFetch
 FetchOptions {
  uri?: string;
  customFetch?: (request: RequestInfo, init: RequestInit) => Promise<Response>;
+  constructOptions?: (requestOrRequests: GraphQLRequest | GraphQLRequest[], options: RequestInit) => RequestInit;


maybe the previous comment can just link to this?

jbaxleyiii · 2017-07-24T13:13:28Z

docs/monorepo.md

+
+## Creating a new fetch
+
+If you are creating a new fetch and would like to included in the lerna repository, the easiest place to start is copying the `apollo-fetch` folder in `<repository root>/packages`.


@stubailo I think this is intended for other contributors to PR and add in new packages if the need arises

jbaxleyiii · 2017-07-24T13:16:16Z

packages/apollo-fetch/src/apollo-fetch.ts

+    });
+  };
+
+  const apolloFetch: ApolloFetch = <ApolloFetch>Object.assign(


@evanshauser what is the purpose of the Object assign here?

jbaxleyiii · 2017-07-24T13:49:01Z

@evanshauser I've added #11 as well to add some root level npm shortcuts and formatting.

fixed spelling errors in tests and json parse error message

0a54994

evans force-pushed the batch-lerna branch 4 times, most recently from dea26a6 to 72a9a0a Compare July 23, 2017 22:22

Evans Hauser added 2 commits July 23, 2017 15:24

lerna support for apollo-fetch as independent package

3e15efc

batching support for apollo-fetch with middleware and afterware

2b75a7b

evans force-pushed the batch-lerna branch from 72a9a0a to 2b75a7b Compare July 23, 2017 22:24

Batched middleware use requests instead of request

430d2a4

evans requested review from stubailo and jbaxleyiii July 24, 2017 05:47

evans changed the title ~~Add lerna support and batching~~ Add lerna support and batched requests Jul 24, 2017

batched request and lerna documentation

cb0e548

evans force-pushed the batch-lerna branch from 05a006c to cb0e548 Compare July 24, 2017 05:54

Updated change log with lerna and batched request support

a4afc5d

stubailo suggested changes Jul 24, 2017

View reviewed changes

jbaxleyiii approved these changes Jul 24, 2017

View reviewed changes

jbaxleyiii mentioned this pull request Jul 24, 2017

run prettier on files #11

Merged

Add clarity around creating a new fetch and batch API

3356d24

stubailo approved these changes Jul 25, 2017

View reviewed changes

stubailo merged commit aea6e08 into master Jul 25, 2017

evans mentioned this pull request Jul 26, 2017

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

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lerna support and batched requests #10

Add lerna support and batched requests #10

evans commented Jul 22, 2017 •

edited

Loading

evans commented Jul 24, 2017

jaydenseric commented Jul 24, 2017

stubailo left a comment

stubailo Jul 24, 2017

stubailo Jul 24, 2017

stubailo Jul 24, 2017

stubailo Jul 24, 2017

stubailo Jul 24, 2017

jnwng Jul 24, 2017

stubailo Jul 24, 2017

jbaxleyiii Jul 24, 2017

jbaxleyiii left a comment

jbaxleyiii Jul 24, 2017

jbaxleyiii Jul 24, 2017

jbaxleyiii Jul 24, 2017

jbaxleyiii Jul 24, 2017

jbaxleyiii Jul 24, 2017

jbaxleyiii commented Jul 24, 2017


		### Batch Formatting

		Since GraphQL does not include batched requests in the formal spec, `apollo-fetch` creates a request that is compatible with [`apollo-server`](https://github.com/apollographql/apollo-server).


		## Creating a new fetch

		If you are creating a new fetch and would like to included in the lerna repository, the easiest place to start is copying the `apollo-fetch` folder in `<repository root>/packages`.


		By default `apollo-fetch` uses `isomorphic-fetch`, but you have the option of using a custom fetch function.
		Additionally, `apollo-fetch` allows specification of how GraphQL requests are included in fetch options.

Add lerna support and batched requests #10

Add lerna support and batched requests #10

Conversation

evans commented Jul 22, 2017 • edited Loading

evans commented Jul 24, 2017

jaydenseric commented Jul 24, 2017

stubailo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaxleyiii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaxleyiii commented Jul 24, 2017

evans commented Jul 22, 2017 •

edited

Loading