W3C Request/Response Normalization #144

dotansimha · 2021-11-17T09:58:20Z

Continue the work from: #94

Fix CI More experiments Better errors ... ... More Fix status code Use babel instead of ts-jest Use ReadableStream polyfill Go for it .. Try out Avoid directory import No more ponyfills No more dependencies deno Update examples Update deno

github-actions · 2021-11-17T09:59:58Z

The latest changes of this PR are available as canary in npm (based on the declared changesets):

[email protected]

github-actions · 2021-11-17T10:00:03Z

❌ Benchmark Failed

Failed assertions detected: some GraphQL operations included in the loadtest are failing.

If the performance regression is expected, please increase the failing threshold.

     ✗ no_errors
      ↳  0% — ✓ 0 / ✗ 16033

     checks.....................: 0.00%   ✓ 0           ✗ 16033
     data_received..............: 0 B     0 B/s
     data_sent..................: 0 B     0 B/s
     http_req_blocked...........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_connecting........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
   ✓ http_req_duration..........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_failed............: 100.00% ✓ 16033       ✗ 0    
     http_req_receiving.........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_sending...........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_tls_handshaking...: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_waiting...........: avg=0s       min=0s       med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_reqs..................: 16033   1602.930177/s
     iteration_duration.........: avg=492.05µs min=180.09µs med=411.69µs max=23.75ms p(90)=547.39µs p(95)=617.53µs
     iterations.................: 16033   1602.930177/s
     vus........................: 1       min=1         max=1  
     vus_max....................: 1       min=1         max=1

packages/core/lib/util/node-http.ts

* Fastify support * Calculate string length * Fix fastify server * Update fastify example * Fix benchmarks * Try sth else * Finalize fastify support * Do not stringify JSON again

vercel · 2021-12-06T10:05:03Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/graphql-helix/graphql-helix/7wfJNjihBXfi8JPjVPeCvAFFneDa
✅ Preview: https://graphql-helix-git-w3c-wip-graphql-helix.vercel.app

vercel · 2021-12-13T12:17:55Z

Deployment failed with the following error:

The most recent charge for your active payment method has failed. Please update it here: https://vercel.com/teams/graphql-helix/settings/billing.

maraisr · 2021-12-17T00:04:56Z

packages/core/lib/get-graphql-parameters.ts

+    case "GET": {
+      operationName = url.searchParams.get("operationName") || undefined;
+      query = url.searchParams.get("query") || undefined;
+      variables = url.searchParams.get("variables") || undefined;
+      break;
+    }


can the post and get avenues be split? Sometimes the GET routes one doesnt want supported.

Isn't that something user can block in their server impl?

they can, but then there is 5 lines of code shipped doing absolutely nothing. Its simple;

getGETHandler(); getPOSTHandler(); gethandleReqRes(); // that calls both.

Helix isnt about making decisions for you, its a "build your own server" library. Making the decision that GET/POST is supported isnt something that is in scope. All the user to "build their own server". Helix should just handle the query in and execution layers, leave the rest up to helpers.

I think separating get/post will break SSE for subscriptions, since it's using GET for the Event-Stream.

See https://stackoverflow.com/questions/34261928/can-server-sent-events-sse-with-eventsource-pass-parameter-by-post

@n1ru4l please confirm?

I generally would still expose that as 2 methods, as some folk won't be using sse. Export and consume what you're going to use.

Actually SSE can be done with both POST and GET. I have been thinking for a while how we could make every functionality of helix more modular.

The HTTP verb specifies how the GraphQL payload sent by the client should be parsed. So on the server we could have some kind of pattern for parsing that:

const payload = await parsePayload({ POST: postParser, GET: getParser })(incomingRequest)

if people want custom behavior or non standard stuff they can simple replace the handler for a given method or remove it.

This approach is more verbose in boilerplate but a bit more modular in giving. A higher level API such as yoga can then provide a opinionated API.

The response protocol is then identified via the content-type and/or accept header, but also the operation type. Today the specification is pretty vague about this, I opened graphql/graphql-over-http#167 a while ago.

We can have a similar API to how we parse the payload for the response protocol:

const response = await matchResponse([ { match: isSSE ,handler: sseResponseBuilder }, { match: isMultiPart, handler: multiPartResponseBuilder }, { match: isApplicationGraphQL, handler: applicationGraphQLResponseBuilder }, // required inputs for determining the response ])(payload, req.headers, req.contentType, streamOrSinglePayloadFromExecute)

@maraisr You can prevent it to parse GET requests by a simple condition. You're still in charge here to decide when the request should be parsed.

if (request.method !== 'GET') { const params = await getGraphQLParameters(request); }

6 lines of code included that won't be used. Death by a thousand cuts. Think optimally.

maraisr · 2021-12-17T00:07:58Z

packages/core/lib/util/calculate-byte-length.ts

@@ -0,0 +1,17 @@
+// returns the byte length of an utf8 string
+export function calculateByteLength(str: string): number {


can just be TextEncoder in browsers/workers and from util on node. No reason to include this.

aka just expose a helix/node-preamble sub-module to do this work for the user.

eg:

const Encoder = new TextEncoder(); export function calculateByteLength(str: string): number { return Encoder.encode(str).byteLength; }

We benchmarked it and it was so slow on Node.js

So the web/browser has to pay the price? chuck it in a preamble or bundle for the different targets.

Actually encoding entire string with TextEncoder sounds more expensive, no?

One would have to benchmark, but in reality who cares about the byte length in the first place? Its a header that nobody is looking at.

Based on my other point in this thread, why does even need to exist in the first instance.

I think nobody refers to the HTTP Client here. As I already said below, some environments return 411 in case of missing length. Also this calculation was already in Helix before so this PR doesn't cover that actually.

maraisr · 2021-12-17T00:09:16Z

packages/core/lib/util/get-response.ts

+    headers: headersInit,
+    status: 200,
+  };
+  const readableStream = new ReadableStream({


ReadableStreams isnt support in workers yet, may i suggest using https://github.com/maraisr/piecemeal here for this.

Could you share a source that shows that? I'd love to see it.

Just try it :)

maraisr · 2021-12-17T00:10:34Z

packages/core/lib/util/get-response.ts

+    "Content-Type": 'multipart/mixed; boundary="-"',
+    "Transfer-Encoding": "chunked",
+  };
+  const responseInit: ResponseInit = {


responseInit should be exposed through an argument, as a user may want to flush extra headers. eg https://github.com/maraisr/piecemeal/blob/2d9121904c33d0d0e6f606752f7250d590e2991b/src/worker.ts#L16-L17

User can do it with response.headers.append method.

Headers, but what about other properties? And fairly sure once a response is created its immutable, or CF throws a hissy fit.

You can copy the parts of the Response object and create your own. Adding this kind of configurations will make this function overcomplicated so I'll keep it in that way.

maraisr · 2021-12-17T00:11:11Z

packages/core/lib/util/get-response.ts

+          const contentLength = calculateByteLength(chunk);
+          const data = [
+            "",
+            "Content-Type: application/json; charset=utf-8",
+            "Content-Length: " + contentLength.toString(),


Suggested change

const contentLength = calculateByteLength(chunk);

const data = [

"",

"Content-Type: application/json; charset=utf-8",

"Content-Length: " + contentLength.toString(),

const data = [

"",

"Content-Type: application/json; charset=utf-8",

content-lengths in multipart segments isnt required, and isnt in the http graphql spec either.

I think it is required in some env.
#145 (comment)

Not in multipart responses, those are effectively "body" and is no mans land. The spec clearly stipulates its not required. Had the exact same learnings during my build of meros.

check https://datatracker.ietf.org/doc/html/rfc1341

It is not spec but the HTTP Client itself. Not required doesn't mean we shouldn't provide it. I think it is good to have this.

Its not good, you have no grounds for that. Multipart isnt part of the http header anymore. Its 100% superficial body.

This is not something this PR covers. This part was already there before this PR.

maraisr · 2021-12-17T00:12:51Z

packages/core/lib/util/node-http.ts

+          new Request(fullUrl, {
+            method: "POST",
+            body: JSON.stringify(maybeParsedBody),
+          }).body,


request.clone().body is gonna be faster, and less code right.

https://developer.mozilla.org/en-US/docs/Web/API/Request/clone

This is actually a hack for NodeJS(just to have the same output with other envs). We don't have body in the request on purpose above because it might be GET request so there is nothing to clone actually.

Why should nodejs be having any impact for consumers of other targets? Apply the "nodejs hacks" for the nodejs target, not for the workers or others.

Granted its in the node utils file.

This is applied already in a NodeJS helper. Check the name of the function, it has NodeRequest :) So other consumers don't call this function at all.

maraisr · 2021-12-23T22:52:35Z

packages/core/lib/util/get-response.ts

+        const encodedChunk = encodeString(chunk);
+        controller.enqueue('\r\n');
+        controller.enqueue('Content-Type: application/json; charset=utf-8\r\n');
+        controller.enqueue('Content-Length: ' + encodedChunk.byteLength + '\r\n');


Again, absolutely 100% zero need or anything for this line to exist. The body isn't being inspect by any cdn or intermediary.

I mean this line is effectively the same as returning json, and the cdn have a whinge coz there was no closing bracket.

It's the body, and those things aren't snooped.

As I already said above, this is not something this PR covers.

dotansimha · 2022-01-05T11:42:00Z

This shared effort to support W3C request/response normalization has been moved to: dotansimha/graphql-yoga#776

The main reason is the fact that this change the nature of Helix and the goals of it (since it introduces a different setup, and aims to provide an agnostic handler for any env, while Helix, at the moment, is node/deno only).

This concept and implementation will land in next major version of Yoga.

dotansimha force-pushed the w3c-wip branch from 5a84b95 to c5a622a Compare November 17, 2021 09:59

dotansimha changed the title ~~WIP: W3C Request Normalization~~ W3C Request/Response Normalization Nov 17, 2021

dotansimha marked this pull request as ready for review November 17, 2021 10:02

n1ru4l reviewed Nov 25, 2021

View reviewed changes

packages/core/lib/util/node-http.ts Outdated Show resolved Hide resolved

n1ru4l reviewed Nov 25, 2021

View reviewed changes

packages/core/lib/util/node-http.ts Outdated Show resolved Hide resolved

ardatan added 2 commits November 25, 2021 15:48

W3C: Fastify support (#177)

f8398c4

* Fastify support * Calculate string length * Fix fastify server * Update fastify example * Fix benchmarks * Try sth else * Finalize fastify support * Do not stringify JSON again

More for W3 like upload, tests for raw http (#195)

ae2a80c

vercel bot deployed to Preview December 6, 2021 10:05 View deployment

Update docs and example for Multipart Request/File Upload (#216)

7fdb2a4

maraisr reviewed Dec 17, 2021

View reviewed changes

Improvements on error handling (#222)

5d49363

maraisr suggested changes Dec 23, 2021

View reviewed changes

dotansimha closed this Jan 5, 2022

		@@ -0,0 +1,17 @@
		// returns the byte length of an utf8 string
		export function calculateByteLength(str: string): number {

W3C Request/Response Normalization #144

W3C Request/Response Normalization #144

Conversation

dotansimha commented Nov 17, 2021 • edited Loading

github-actions bot commented Nov 17, 2021 • edited Loading

github-actions bot commented Nov 17, 2021 • edited Loading

❌ Benchmark Failed

vercel bot commented Dec 6, 2021 • edited Loading

vercel bot commented Dec 13, 2021

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

maraisr Dec 17, 2021 • 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

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

maraisr Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

ardatan Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotansimha commented Jan 5, 2022 • edited Loading

dotansimha commented Nov 17, 2021 •

edited

Loading

github-actions bot commented Nov 17, 2021 •

edited

Loading

github-actions bot commented Nov 17, 2021 •

edited

Loading

vercel bot commented Dec 6, 2021 •

edited

Loading

maraisr Dec 17, 2021 •

edited

Loading

maraisr Dec 17, 2021 •

edited

Loading

ardatan Dec 17, 2021 •

edited

Loading

dotansimha commented Jan 5, 2022 •

edited

Loading