spec-compliant GET support #490

glasser · 2018-02-16T20:25:03Z

Review commits separately. In addition to explicitly supporting GETs in apollo-link-http, we ban GETs in apollo-link-batch-http, and add an Observable helper fromError.

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

apollo-cla · 2018-02-16T20:30:26Z

	Messages
📖	Please add your name and email to the AUTHORS file (optional)
📖	If this was a change that affects the external API, please update the docs and post a link to the PR in the discussion

Generated by 🚫 dangerJS

codecov-io · 2018-02-16T20:32:35Z

Codecov Report

Merging #490 into master will decrease coverage by 0.04%.
The diff coverage is 90.9%.

@@            Coverage Diff            @@
##           master    #490      +/-   ##
=========================================
- Coverage   91.35%   91.3%   -0.05%     
=========================================
  Files          19      19              
  Lines         798     828      +30     
  Branches      196     204       +8     
=========================================
+ Hits          729     756      +27     
- Misses         64      67       +3     
  Partials        5       5

Impacted Files	Coverage Δ
packages/apollo-link-retry/src/retryFunction.ts	`100% <ø> (ø)`	⬆️
packages/apollo-link-retry/src/delayFunction.ts	`100% <ø> (ø)`	⬆️
packages/apollo-link/src/linkUtils.ts	`89.04% <100%> (+0.46%)`	⬆️
packages/apollo-link-http-common/src/index.ts	`85.89% <100%> (ø)`	⬆️
...ckages/apollo-link-batch-http/src/batchHttpLink.ts	`91.54% <83.33%> (-1.31%)`	⬇️
packages/apollo-link-http/src/httpLink.ts	`90.47% <91.66%> (+0.82%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d257ade...fffde25. Read the comment docs.

glasser · 2018-02-16T23:38:10Z

Hmm, both the template and the bot tell me to update AUTHORS. But does this repo actually have AUTHORS?

They also refer to making and linking to docs PRs, but for this repo docs are inside the repo, right?

evans

Looks really good! fromError is great. A couple documentation suggestions. Otherwise looks good to me.

evans · 2018-02-16T23:42:56Z

packages/apollo-link-batch-http/README.md

-  fetch: customFetch
-});
-```
+You can use the `fetch` option when creating an http-link to do a lot of custom networking. This is useful if you want to modify the request based on the headers calculated or calculate the uri based on the operation:


I think "request based on the calculated headers or calculate" would be better

evans · 2018-02-16T23:49:30Z

packages/apollo-link-http-common/src/index.ts

@@ -44,6 +44,14 @@ export interface UriFunction {
  (operation: Operation): string;
 }

+// The body of a GraphQL-over-HTTP-POST request.
+export interface Body {


We may be able to have Body be an interface that extends Partial, see this SO. GraphQLRequest is exported from apollo-link

query is a string rather than a DocumentNode (and there's no context), so I don't think that'll work?

In my experience over the entire open source and commercial Apollo codebase, guessing what type is going to be in a field or variable called query is the most challenging part :)

evans · 2018-02-16T23:51:11Z

packages/apollo-link-http/README.md

-  fetch: customFetch
-});
-```
+You can use the `fetch` option when creating an http-link to do a lot of custom networking. This is useful if you want to modify the request based on the headers calculated  or calculate the uri based on the operation:


calculated headers sounds a bit better in my head

evans · 2018-02-16T23:52:40Z

packages/apollo-link-http/src/__tests__/httpLink.ts

-  fetchMock.post('begin:data', makePromise(data));
+    beforeEach(() => {
+      fetchMock.restore();
+      fetchMock.post('begin:http://data/', makePromise(data));


the uri's are nice! Much more descriptive

Heh, back when I implemented this using the URL web API it didn't work without this :)

evans · 2018-02-17T00:13:25Z

packages/apollo-link-http-common/src/index.ts

@@ -233,18 +241,18 @@ export const selectHttpOptionsAndBody = (
  };
 };

-export const serializeFetchBody = body => {
-  let serializedBody;
+export const serializeFetchParameter = (p, label) => {


@jaydenseric This is going to be a breaking change for [email protected]

Is this OK? I thought this hadn't been released yet so it was an OK interface to break without documenting.

It's ok if the change is documented in a changelog entry, and a new version of apollo-link-http-common is published with a semver major (breaking change).

Ah, I didn't realize Evans had published a non alpha version of the package.

Evans, should I just revert the name change or something, or bump to 0.2.0?

If you keep the breaking changes, please bump to v1.0.0, breaking changes in semver minor releases are the devil.

@glasser We can bump to v1.0.0, since it will make Jayden's life easier. I'm not worried about having the version number bloat and it might force better practice. The plan for apollo-link-common-http was to be a semi private API(without documentation due to overhead) that developers of links can look at if they want to align with apollo-link-http and apollo-link-batch-http. In that goal I think attempting to follow SemVer would be make keeping up with the changes easier

Sorry I am wrong ^. Let's do v0.2, since we are pre v1.0 SemVer says that a breaking change should be a minor version. Also it looks like we should be okay for Jayden's upload-client

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you’re worrying a lot about backwards compatibility, you should probably already be 1.0.0.

All 3 things should be assumed to be true for every npm package that can be publically installed. There are only upsides and no downsides to bumping the major version number if there is ever a breaking change.

FB in particular is the absolute worst for this. react jumping from v0.14 to v15 was an admission of guilt they had it wrong for 14 major versions. graphql being a v0.x causes great disruption to the ecosystem every update.

glasser · 2018-02-17T08:24:34Z

OK, I bumped the version of apollo-link-http-common to 0.2.0. This should be ready to merge (please don't squash). @evans / @jbaxleyiii I'll let you do the merge — @evans said you were trying to get another release out soon and it would be great to get this in it.

There is no spec for batching over GET. I don't believe apollo-server supports it given that there's no spec. We can always come up with a spec and implement it later.

Fixes #236.

glasser changed the title ~~[wip] apollo-link-batch-http: disallow GET with batches~~ [wip] spec-compliant GET support Feb 16, 2018

glasser force-pushed the glasser/better-GET branch from cebff49 to da22c35 Compare February 16, 2018 20:27

glasser force-pushed the glasser/better-GET branch from 600bc69 to fe3aa2f Compare February 16, 2018 23:26

glasser requested a review from evans February 16, 2018 23:37

evans reviewed Feb 17, 2018

View reviewed changes

glasser changed the title ~~[wip] spec-compliant GET support~~ spec-compliant GET support Feb 17, 2018

glasser force-pushed the glasser/better-GET branch 2 times, most recently from 4b4c63f to 30539b1 Compare February 17, 2018 08:24

glasser added 5 commits February 17, 2018 12:05

apollo-link-batch-http: disallow GET with batches

ad49e93

There is no spec for batching over GET. I don't believe apollo-server supports it given that there's no spec. We can always come up with a spec and implement it later.

npm run lint-fix

ddc0f14

apollo-link: new fromError helper

ec5a059

apollo-link-http: support GET requests natively

95caecf

Fixes #236.

apollo-link-http-common: bump version

fffde25

evans force-pushed the glasser/better-GET branch from 30539b1 to fffde25 Compare February 17, 2018 20:08

evans merged commit 80bb129 into master Feb 17, 2018

evans deleted the glasser/better-GET branch February 17, 2018 20:44

glasser mentioned this pull request May 5, 2022

Add a comment about a new use of the content-type header apollographql/apollo-client#9683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spec-compliant GET support #490

spec-compliant GET support #490

glasser commented Feb 16, 2018 •

edited

Loading

apollo-cla commented Feb 16, 2018 •

edited

Loading

codecov-io commented Feb 16, 2018 •

edited

Loading

glasser commented Feb 16, 2018

evans left a comment

evans Feb 16, 2018

evans Feb 16, 2018

glasser Feb 17, 2018

evans Feb 16, 2018

evans Feb 16, 2018

glasser Feb 17, 2018

evans Feb 17, 2018 •

edited

Loading

glasser Feb 17, 2018

jaydenseric Feb 17, 2018 •

edited

Loading

glasser Feb 17, 2018

glasser Feb 17, 2018

jaydenseric Feb 17, 2018

evans Feb 17, 2018

evans Feb 17, 2018

jaydenseric Feb 17, 2018

glasser commented Feb 17, 2018 •

edited

Loading

spec-compliant GET support #490

spec-compliant GET support #490

Conversation

glasser commented Feb 16, 2018 • edited Loading

apollo-cla commented Feb 16, 2018 • edited Loading

codecov-io commented Feb 16, 2018 • edited Loading

Codecov Report

glasser commented Feb 16, 2018

evans 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

evans Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaydenseric Feb 17, 2018 • edited Loading

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

glasser commented Feb 17, 2018 • edited Loading

glasser commented Feb 16, 2018 •

edited

Loading

apollo-cla commented Feb 16, 2018 •

edited

Loading

codecov-io commented Feb 16, 2018 •

edited

Loading

evans Feb 17, 2018 •

edited

Loading

jaydenseric Feb 17, 2018 •

edited

Loading

glasser commented Feb 17, 2018 •

edited

Loading