Batching: adds support for maxBatchSize option

[brad-decker]

As discussed in #39 there are certain situations where a user may want to limit the amount of requests sent to the batchUrl at once.

This PR changes the batched request workflow so that if maxBatchSize is provided it will chunk out the batched requests into numerous requests, the flatten the responses into a single array and resolve them the same way relay-network-layer already does.

Some utility functions were added to help stay DRY and limit the amount of looping necessary when no maxBatchSize is provided.

[brad-decker]

@nodkz haven't wrote tests yet. Wondering if you want me to extend the current batchQueries test file or create a new one for maxBatchSize. Will need to create a new instance of Relay Network Layer with that option set so it might make sense to copy the structure of the tests in that file but issue them with the batch size.

Looking forward to your feedback!

[nodkz]

Cool!! Will try to review it tomorrow. From mobile now.

[nodkz]

Impressive! Good job! 👍

I'm ready to merge and publish your current code without tests and small improvements if it's blocking development of your app. If not, we can make some improvements before shipping it to npm.

[brad-decker]

Let's work on the improvements and ship it when we're both happy with it. 😃👍 thanks

[nodkz]

Suggested improvements:

• [x] 1) add tests in batch.test.js

with sub section describe('maxBatchSize option', () => ...) and defining into it rnl2 with chunck option (rnl3 and so on if needed). Cause current test file are not so big and tests logic of one source file, I do not see the point of creating another file.

---

• [x] 2) rewrite a little bit logic with getting rid of masterRequestMap

We should not gather all map into one object, cause if some request in one chunk will be rejected will be rejected all requests from all chunks. But better to reject requests just from one chunk. So we can rewrite resolveBatchedQueries it in such manner:

- resolveBatchedQueries(fetchWithMiddleware, relayRequestList, requests, requestMap);
+ resolveBatchedQueries(fetchWithMiddleware, [ { req, requestMap}, {}, ... ])`. 

No need in relayRequestList, requestMap contains needed requests already. And in catch requests may be rejected from requestMap.

---

• [x] 3) rename maxBatchSize to maxBatchRequestNum or something shorter and more obvious.

Cause better to keep maxBatchSize option name for size in utf8 symbols (or kilobytes) not to query nums.

BTW we can calculate size of our batchRequest if add requestMap[id].body=JSON.stringify({...}) and in buildRelayRequest() method prepare body in such manner

const reqs = Object.keys(requestMap).map(id => requestMap[id].body).join(',');
req.body = `[${reqs}]`; 

We can automatically split big chunks by maxBatchSize in symbols with default value around 50176 (cause utf8_string.length in most cases return in 2 times less value (in symbols) which express-graphql expects (in bytes, max 100kb)). And of course provide ability to change this value via options.

Chunking by size is the much better solution, cause some of the queries may be big, other small. So it will be difficult to find optimal request number per chunk. Anyway we can keep both options maxBatchSize and maxBatchRequestNum.

[brad-decker]

All of this sounds great, @nodkz - i actually prefer splitting by the size of request instead of some number of requests so we don't even need to keep that option. I am having some problems figuring out the right way to split the requests by size.

If we have a requestMap that has the body of each request contained within it, is the correct approach to keep adding relayRequests into the current chunk until the total body size would break over the maxBatchSize and at that point start a new chunk? Or is there some magic way of splitting a request by size ( you said (in symbols) which i'm not 100% on ).

i'll keep working on it just wanted to make sure i had a good idea of your overall vision before i get too deep into this.

[nodkz]

From mobile, so shortly.

reqMap.body.length will return number of symbols of utf8 string, not real size in bytes, which will be checked by express-graphql. So we should keep it in mind and provide correct default value 1024*1024/2-some size for service symbols.

No special magic with splitting. Your logic is correct. 👍

Just one notice for edge case: if received​ request which size is greater than maxBatchSize, we should send it anyway to server without batching.

[nodkz]

@brad-decker please close this PR and make new one according to new branch batchMW.

I decide that with you PR we can bump new major version 2.0.0. So I completely rewrite batch logic, now it is middleware. I have long wanted to do it, and at last, you motivate me with your PR.

With new code will be much simpler to add splitting of batched requests. Your place to put splitting is here https://github.com/nodkz/react-relay-network-layer/blob/batchMW/src/middleware/batch.js#L32-L35:

  // here we can check batcher body size
  // and if needed splitting - create new batcher:
  //   singleton.batcher = prepareNewBatcher(next, opts);
  singleton.batcher.bodySize += req.body.length;

Also updated tests. You may add testing of maxBatchSize option like it's done with batchTimeout option (new feature): https://github.com/nodkz/react-relay-network-layer/blob/batchMW/test/batch.test.js#L121-L192

So for now, your task became much simpler:

• add maxBatchSize option to readme, implement logic and add tests 😉

[brad-decker]

Thanks @nodkz the new work looks awesome. I'll add my logic in in a new PR and ping you.

[nodkz]

Thank you for understanding. It was a necessary action due to accumulated technical debt.

[brad-decker]

when packing this and installing into our application every query fails.

js Warning: RelayReadyState: Invalid state change from `{"aborted":false,"done":false,"error":{"response":{"viewer": [....cut for brevity] to `{"error":{"response":{"viewer":{"id":"Vmlld2VyOg=="}}}}

followed by an "uncaught" error matching the shape of the payload sent to request.resolve. any ideas?

[nodkz]

Ups. Means something broken. Didn't test it with my app. Will do it tomorrow, out of work now.

Can you copy here from network tab request body and server response? Or try to resolve bug yourself. Or drop it till tomorrow.

Sorry for inconvenience.

[brad-decker]

@nodkz figured out the issue. When you use Object.assign with batchResponse it doesn't copy over anything. I guess all the properties of batchResponse are enumerable set to false?

batchResponse.json.forEach(function (res) {
        if (!res) return;
        var request = requestMap[res.id];
        if (request) {
          if (res.payload) {
            request.done = true;
            const mockRes = {};
            mockRes.json = res.payload;
            mockRes.ok = batchResponse.ok;
            mockRes.status = batchResponse.status;
            request.resolve(mockRes);
            return;
          }
          // compatibility with graphene-django and apollo-server batch format
          request.done = true;
          request.resolve(Object.assign({}, batchResponse, { json: res }));
        }
      });

this was the minimum required fields to get a successful load. Do you have a preferred method of dealing with this? I could create a simple function that creates a object payload with those fields based off a batched response and use that to send to request.resolve

[brad-decker]

Also just for my own knowledge can you point me in the right direction to find what request.resolve actually does in Relay source? I see Relay source has a RelayQuery class that extends Deferrable but i'm not familiar with that or the package. Would love to understand the underlying process a little more.

[brad-decker]

moved to #41

[nodkz]

Good catch! Completely forgot that Response is an internal object and can have different implementation depending on browser/environment. That's why test passed, but on prod fails. Thanks for resolving this.

---

About request.resolve:

Let's take https://facebook.github.io/relay/docs/interfaces-relay-network-layer.html#sendqueries as example:

sendQueries(queryRequests) {
  return Promise.all(queryRequests.map(
    queryRequest => fetch(...).then(result => {
      if (result.errors) {
        queryRequest.reject(new Error(...));
      } else {
        queryRequest.resolve({response: result.data});
      }
    })
  ));
}

sendQueries method can take several queries in one time. To have ability resolve each query individually as soon as it fetched from the server. Other solution can be: wait while fetched all queries, and only then populate RelayStorage. So for performance reason, they choose the first solution and expose resolve/reject methods for every queryRequest. But keep overall Promise for sendQueries for catching errors and some internal propagation.

Also I suppose that they use websockets in their internal apps instead of http. Or maybe mix of websockets/http. So their build-in NetworkLayer is for outside developers or tests.

About Deferrable can not say anything, I do not dig so deeply.