http-fetcher with basic tests #5

evans · 2017-06-15T07:01:01Z

This includes the most basic fetcher, http-fetcher with a couple of tests. There are a couple constructs that illustrate possible additions to the specification and a bunch of notes for additions that need to be made to make this http-fetcher a canonical example.

helfer

This looks great @evanshauser! I left quite a few comments, but most of them are minor. 🙂
Before we merge this in, we should also add at least one test with a mutation, which should be sent via POST. I'd also say that it should actually be configurable whether queries are sent over GET, but let's leave that for the future (maybe just leave a comment in the code about that). The reason for not wanting to send over GET sometimes is because many servers don't allow query strings to have more than 2000 characters or so.

helfer · 2017-06-16T00:30:39Z

package.json

@@ -32,16 +32,21 @@
    "clean": "npm run clean:dist && npm run clean:coverage",
    "clean:dist": "rimraf dist/*",
    "clean:coverage": "rimraf coverage/*",
-    "prepublish": "npm run clean && npm run build"
+    "prepublishOnly": "npm run clean && npm run build"


What does this do? Is this a new npm thing?

helfer · 2017-06-16T00:31:13Z

src/httpFetcher.ts

@@ -0,0 +1,175 @@
+import { ApolloFetcher, Observable, FetchResult, Subscriber, Operation, State } from './types';


We usually break these over multiple lines if there's more than 2 or so.

helfer · 2017-06-16T00:31:21Z

src/httpFetcher.ts

+import { ApolloFetcher, Observable, FetchResult, Subscriber, Operation, State } from './types';
+import { toSubscriber } from './subscriber';
+import { print } from 'graphql';
+import { DocumentNode, DefinitionNode, OperationDefinitionNode } from 'graphql/language/ast';


Same here, multiple lines.

helfer · 2017-06-16T00:33:01Z

src/httpFetcher.ts

+
+  public start() {
+    if (this.state !== State.COLD) {
+      throw Error('Observer already started');


I don't know if we should really throw here. It might be better to just do nothing.

helfer · 2017-06-16T00:33:45Z

src/httpFetcher.ts

+  }
+
+  public stop() {
+    if (this.state === State.COMPLETED || this.state === State.ERRORED) {


Again, I'd just ignore it if we already stopped. That will make the operation idempotent which is easier to reason about.

helfer · 2017-06-16T00:58:47Z

src/subscriber.ts

+import { Subscriber } from './types';
+
+function isSubscriber<T>(object: any): object is Subscriber<T> {
+  return 'next' in object;


Is next always required for a subscriber? Or could it just have error? Maybe the safer thing to check would be to see if it's a function and if it is, treat it as the next function.

helfer · 2017-06-16T00:59:42Z

src/types.ts

+}
+
+export interface Operation {
+  query: DocumentNode;


This is good for now, but I think eventually we'll also allow sending just the operation name (for persisted queries). Speaking of persisted queries, that's a use-case we should add to our list!!!

helfer · 2017-06-16T01:02:57Z

src/types.ts

+  context?: object;
+}
+
+export type UnsubscribeHandler = () => void;


I think this is now different in the TC39 proposal. Do we want to try to align here?

helfer · 2017-06-16T01:04:29Z

tests/index.ts

+    observable.subscribe({
+      next,
+      error: (error) => assert(false),
+      complete: () => { assert(next.calledOnce); done(); },


Have you tried if this test can fail at all?

helfer · 2017-06-16T01:06:07Z

tests/index.ts

+    const next = sinon.spy();
+    const data = {hello: 'world'};
+    fetchMock.get('*', data);
+    const fetcher = new HttpFetcher('', fetch);


We should make the fetch argument optional. We'll add a separate test for adding the argument later, which can use sinon.spy to check that the function is getting called.

…Fetcher Interface

…atch

evans · 2017-07-06T00:50:31Z

src/link.ts

+  }
+
+  private static buildLinkChain(links: ApolloLink[]): NextLink {
+    const _links = [...links];


This way of creating the connections between the links makes a new array for each request. It mirrors the middleware and afterware code from the netowrk interface of apollo-client(also in apollo-fetch) the next function on demand. We could create a closure and keep an index that we increment

…ents

…on each other

…mpatibility

jbaxleyiii

@evanshauser I think we are missing some clarity around how forward works / what the user has to return. Lets chat more about this

jbaxleyiii · 2017-07-14T05:19:09Z

src/setContextLink.ts

+    super();
+  }
+
+  public request(operation: Operation, forward: NextLink): Observable<FetchResult> {


@evanshauser I'm concerned about the current forward pattern since it could be optional? I'm a little confused at forward and how we even do things like enforcing passing a forward into a forward?

jbaxleyiii · 2017-07-14T05:19:30Z

src/link.ts

+  if (operation.variables === undefined) {
+    operation.variables = {};
+  }
+  if (operation.query === undefined) {


what is this for?

jbaxleyiii · 2017-07-14T05:19:51Z

src/link.ts

+  }
+  const _operation = transformOperation(operation);
+
+  return link.request(_operation) || Observable.of();


should there be a forward passed to this request?

…mpty args are terminating

stubailo

A few general concerns:

A good number of unused files - we can move them to some folder, or remove them entirely
We aren't consistently using the utilities we define, like toLink. We should make sure to use them everywhere.

stubailo · 2017-07-17T18:30:56Z

src/cacheLink.ts

@@ -0,0 +1,44 @@
+import {


This file is unused - can we move it into the tests, or an unused folder, or remove it?

I'd say lets remove it for now as to not confuse early adopters with a potential spec for caching

stubailo · 2017-07-17T18:33:38Z

src/httpLink.ts

+  }
+
+  public request(operation: Operation): Observable<FetchResult> | null {
+    this.headers = operation.context && operation.context.headers || {};


For every link, we should make sure to document what it expects on the context.

Agreed, until we settle on a formal contextTypes kind of API (and on improving the types), I think we could just note it above the class definition as inline docs?

stubailo · 2017-07-17T18:34:21Z

src/httpUtils.ts

@@ -0,0 +1,46 @@
+import {


This file is not used. Let's remove it?

stubailo · 2017-07-17T18:35:06Z

src/link.ts

+    operation.variables = {};
+  }
+  if (!operation.query) {
+    console.warn(`query should either be a sting or GraphQL AST`);


sting -> string

stubailo · 2017-07-17T18:35:34Z

src/link.ts

+    operation.query = <DocumentNode>{};
+  }
+
+  const _operation: Operation = transformOperation(operation);


Maybe transformedOperation? _operation isn't a super descriptive name.

lets just move transformOperation into the request call:

return link.request(transformOperation(operation)) || Observable.of();

stubailo · 2017-07-17T18:44:28Z

src/linkUtils.ts

@@ -0,0 +1,41 @@
+import {


Should we move toLink, isTerminating, etc, here?

yes please!

stubailo · 2017-07-17T18:45:51Z

src/mockLink.ts

@@ -0,0 +1,22 @@
+import {


This is not used

stubailo · 2017-07-17T18:47:31Z

src/pollingLink.ts

+
+  constructor(pollInterval?: number) {
+    super();
+    this.pollInterval = pollInterval || 0;


I'd say we should just throw if the poll interval is not set. 0 doesn't seem like a reasonable default.

stubailo · 2017-07-17T18:47:52Z

src/pollingLink.ts

+
+      const poll = (() => {
+        forward(operation).subscribe(subscriber);
+      }).bind(this);


I don't think the bind here is necessary.

stubailo · 2017-07-17T18:48:31Z

src/retryLink.ts

+          observer.complete();
+        },
+        // This causes an error, not sure why
+        // complete: observer.complete


You probably need observer.complete.bind(observer).

evans · 2017-07-17T19:08:28Z

src/pollingLink.ts

+
+  constructor(pollInterval?: number) {
+    super();
+    this.pollInterval = pollInterval || 0;


should be interval

@evanshauser are you open to it being a function that given the operation, can determine the interval?

Sounds good!

evans · 2017-07-17T19:30:59Z

src/setContextLink.ts

+    if (!operation.context) {
+      operation.context = {};
+    }
+    operation.context = {


This should be a recursive merge, since a link could overwrite an entire object, such as headers

fwiw, recursive merges are really expensive :( I would say that this link overwrites and previous context of the same keys when being added.

@evanshauser when we call request within the ApolloLink interface, thoughts on creating a copy of the operation and passing it down?

jbaxleyiii

@evanshauser this is getting close! A few changes / question in here.

I would also really like to see some inline documentation above methods on the Link abstract. That way when we start iterating on it, we can make sure we stick / understand the original decisions!

Great stuff!

jbaxleyiii · 2017-07-17T19:41:22Z

src/cacheLink.ts

@@ -0,0 +1,44 @@
+import {


I'd say lets remove it for now as to not confuse early adopters with a potential spec for caching

jbaxleyiii · 2017-07-17T19:42:34Z

src/httpLink.ts

+  }
+
+  public request(operation: Operation): Observable<FetchResult> | null {
+    this.headers = operation.context && operation.context.headers || {};


Agreed, until we settle on a formal contextTypes kind of API (and on improving the types), I think we could just note it above the class definition as inline docs?

jbaxleyiii · 2017-07-17T19:45:13Z

src/link.ts

+      return ApolloLink.empty();
+    }
+
+    return links


@evanshauser this is nitpicky (and totally not necessary to change), but with reduce you can specify the starting point.

i.e.

links.map(toLink).reduce((x, y) => x.concat(y)), ApolloLink.empty())

@jbaxleyiii The issue I ran into was that reduce is applied, so that the initial argument is the first in the chain, so the function calls became:

ApolloLink.empty().concat(first).concat(second)...

We could do an ApolloLink.passthrough() as the initial value

ah shoot, yep! thank you! Lets keep it as is

jbaxleyiii · 2017-07-17T19:48:11Z

src/link.ts

+  // join two Links together
+  public concat(link: ApolloLink | RequestHandler): ApolloLink {
+    if (this.request.length <= 1) {
+      const warning = Object.assign(


should we also use isTerminating(this) to be consistent in checking?

jbaxleyiii · 2017-07-17T19:49:09Z

src/link.ts

+    }
+    link = ApolloLink.toLink(link);
+
+    return link.request.length <= 1 ? new TerminatedConcat(this, link) : new ConcatLink(this, link);


Same question here about using the method instead of rewriting the check

jbaxleyiii · 2017-07-17T20:22:41Z

tests/httpLink.ts

+      error: (error) => assert(false),
+      complete: () => {
+        const body = JSON.parse(fetchMock.lastCall()[1]['body']);
+        assert.equal(body['query'], print(sampleMutation));


any reason for not using dot notation here?

jbaxleyiii · 2017-07-17T20:24:46Z

tests/link.ts

+  describe('concat', () => {
+    it('should concat a function', (done) => {
+      const returnOne = new SetContextLink({ add: 1 });
+      const link = returnOne.concat((operation, forward) => Observable.of({ data: operation.context.add }));


@evanshauser this shows my potential fear with forward :(

I'm assuming the library would not see this as a terminating link, yet forward is not used so it in essence is terminating. I think with copy paste / boilerplate, this could really get people confused

jbaxleyiii · 2017-07-17T20:25:47Z

tests/link.ts

+        return forward(operation);
+      });
+      const mock2 = new MockLink((op, forward) => Observable.of({ data: op.context.add + 2 }));
+      const mock3 = new MockLink((op, forward) => Observable.of({ data: op.context.add + 3 }));


lets move MockLink to the /tests folder so it won't be confused as an external link

jbaxleyiii · 2017-07-17T20:28:33Z

tests/singleRequestLink.ts

+import SingleRequestLink from '../src/singleRequestLink';
+
+
+describe('SingleRequestLink', () => {


I don't think this is needed since the link won't be shipped. If it is, this needs a lot more tests

jbaxleyiii · 2017-07-17T20:29:14Z

tests/tests.ts

@@ -0,0 +1,7 @@
+import './httpLink';


Is this file needed? Can you not glob test?

…ved SingleRequestLink

jbaxleyiii · 2017-07-18T19:40:44Z

src/link.ts

+      return ApolloLink.empty();
+    }
+
+    return links


ah shoot, yep! thank you! Lets keep it as is

Evans Hauser added 2 commits June 14, 2017 23:30

http-fetcher with basic tests

8ce09a0

Add http-fetcher tests for coverage

56fe476

evans requested a review from helfer June 15, 2017 07:01

helfer reviewed Jun 16, 2017

View reviewed changes

Evans Hauser added 11 commits June 16, 2017 10:41

Added more tests to increase coverage and fixed unsubscribe edge case

4c9dd4c

moved Observable behavior into abstractObservable

9e3b839

Reorganized tests into separate files

833baac

Add initial authHttpFetcher to show authentication

59380c6

Moved httpFetcher into utils and Observable now abstract

edc736a

test for authHttpFetcher for coverage

76dcbe7

fixed auth test and syntax error in helpers

38ee6ee

rename package to apollo-fetchers

f38a1ec

Add Promise wrapper for Http fetcher

81e7776

0.0.1 package version

95a6448

npm test and made fetcher-as-promise conform to NetworkInterface and …

15fc225

…Fetcher Interface

apollographql deleted a comment from coveralls Jun 21, 2017

Evans Hauser added 8 commits June 23, 2017 17:54

Add watcher problem matcher

4a9d493

Observable now continue when out of order functions called

0277a3d

split network interface and promise wrappers

87f6146

httpFetcher now uses promise internally and reorganized Utils

88e35ea

include batching fetcher and update index

9bf7255

tests for http fetchers faster, need reorg

4020705

Organized batchHttp tests

99532cc

changed name to apollo-link and added apollo-fetch to httpLink

55b4502

helfer mentioned this pull request Jul 4, 2017

Feature idea: Combining middleware and afterware apollographql/apollo-client#1852

Closed

Evans Hauser added 3 commits July 5, 2017 10:19

Removed batching, since it will be inside of apollo-fetch or apollo-b…

7e7ec1b

…atch

Add retry link with tests

d0918b2

Added static Link library with chain function and some tests(needs more)

e32bce5

evans commented Jul 6, 2017

View reviewed changes

Add more tests, HttpLink alias, and subscriber can have null next

23e08b5

Evans Hauser added 8 commits July 11, 2017 08:54

Polling Link example untested

b7d6228

Concat and split on static library still contain mutable code as comm…

abf7a55

…ents

Transition to ApolloLink as Abstract class with split and concat

8559ced

more testing and added pollingLink

85216d8

Fixed httpLink request signature

f1c6484

removed use-before-declare, since ApolloLink and FunctionLink depend …

8631e9f

…on each other

Made external operation into GraphQLRequest and query optional for co…

c39c465

…mpatibility

Add graphql types and fixed associated typing

4aff934

jbaxleyiii suggested changes Jul 14, 2017

View reviewed changes

Evans Hauser added 6 commits July 13, 2017 23:00

split is now static and instance method of ApolloLink

af2fb3a

exporting execute and ApolloLink instead of Links

262fb1a

Remove unnecessary type exports

2db54d4

Warns when concating to a on terminating link

0b8c187

spliting a terminating split test

1ef4b87

variables and context are objects and checking to ensure links with e…

9e5a7dd

…mpty args are terminating

stubailo suggested changes Jul 17, 2017

View reviewed changes

evans commented Jul 17, 2017

View reviewed changes

jbaxleyiii suggested changes Jul 17, 2017

View reviewed changes

Evans Hauser added 6 commits July 17, 2017 18:21

removed external request calls, Split/ConcatLink, updated types, remo…

f91cf05

…ved SingleRequestLink

made bracket notation into dot

66b0ec0

Link validation and execute makes a copy of operation

e421a8a

Polling interval function contains types

3a1f478

Add context note to httpLink

ac98087

bump package number and add jon as contributor

523d716

evans force-pushed the http-fetcher branch from bdf6960 to 523d716 Compare July 18, 2017 17:21

jbaxleyiii approved these changes Jul 18, 2017

View reviewed changes

src/link.ts

return ApolloLink.empty();

}

return links

Copy link

Contributor

jbaxleyiii Jul 18, 2017

Choose a reason for hiding this comment

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

ah shoot, yep! thank you! Lets keep it as is

Merge branch 'master' into http-fetcher

42078bd

jbaxleyiii merged commit 2cd697a into master Jul 18, 2017

jbaxleyiii deleted the http-fetcher branch July 18, 2017 19:49

		@@ -0,0 +1,175 @@
		import { ApolloFetcher, Observable, FetchResult, Subscriber, Operation, State } from './types';

		import SingleRequestLink from '../src/singleRequestLink';


		describe('SingleRequestLink', () => {

http-fetcher with basic tests #5

http-fetcher with basic tests #5

Conversation

evans commented Jun 15, 2017 • edited by cshaw4

helfer 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

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

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

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

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

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

evans commented Jun 15, 2017 •

edited by cshaw4