Run Middleware/Afterware once on batchedNetworkInterface

[lewisf]

Addresses #1191.

[apollo-cla]

@lewisf: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[lewisf]

@helfer can i get some feedback on the batchrequest types and usage here? i'm not super familiar with typescript and i'm not sure if this is the right way to do it, thanks!

hoping to see that the implementation details look good before adding to the docs and stuff :)

[helfer]

Thanks @lewisf! I think the proper way to do it in Typescript would be to keep middleware for normal network interfaces and middleware for the batchNetworkInterface separate. One takes a RequestAndOptions, the other one takes a BatchRequestAndOptions. In order to do that, you'll have to define separate use and useAfter functions for the batchNetworkInterface (although you can reuse most of the code). The most important thing is that people using middleware or afterware with Typescript get an error if they try to apply non-batched afterware to a BatchedNetworkInterface.

I hope that makes sense. If not, let me know!

[helfer]

I don't know if this would work, but I don't think it's advisable anyway because it makes type checks afterwards impossible. Instead of doing this (and the same thing for afterwares) I would redefine the afterwares signature for the batchNetworkInterface by overriding the use and useAfter methods to take a batchMiddleware or batchAfterware instead (which you have to write).

[lewisf]

okay cool, makes sense. i'll give that a shot

[lewisf]

@helfer thanks for the help so far.

i started looking into redefining use/useAfter in batchedNetworkInterface but ran into typing issues.

it looks like in order to overload use and useAfter, i'm going to have to use user-defined typeguards, in which case we'd have to be able to differentiate between batch/non-batch wares.

would it be reasonable to define the signature of batch wares to be applyBatchMiddleware/applyBatchAfterware, or continue to use applyMiddleware/applyAfterware but give those wares a property, like batch: true?

also i'm trying to decipher this comment here:

apollo-client/src/transport/networkInterface.ts

Lines 89 to 90 in 3d9b44d

	// TODO: refactor
	// add the batching to this.

should i be concerned about that comment?

[helfer]

@lewisf

I think it's perfectly fine to call it applyBatchMiddleware. It's even a bit cleaner, because it makes it clear that this middleware isn't the exactly the same.

PS: You don't have to worry about that comment, if you want you can actually remove it.

[lewisf]

@helfer i don't really like what im doing here with overloading + user defined type guards, but this is a public api on network interfaces that im not comfortable changing for the batched network interface. thoughts?

[helfer]

@lewisf could you maybe just define the use for BatchedRequests on the HTTPBatchedNetworkInterface? If it's not possible to override the method there, then maybe we can make both the HTTPFetchNetworkInterface and the batch network interface inherit from a common ancestor and separately define use. What do you think?

[lewisf]

@helfer that sounds reasonable to me.

I like the approach of also making HTTPNetworkInterface parameterized to type of middleware and type of afterware? that would give us the ability to also parameterize use and useAfter. here's a working implementation with what i mean: master...lewisf:lewis/batch-single-run-wares-parameterized. However, doing this might make it difficult to define a queryMiddleware for HTTPBatchedNetworkInterface in the future though if that's still on the table.

Alternatively, I can greatly lax the HTTPNetworkInterface by removing use/useAfter and associated properties, or laxing their types to MiddlewareInterface[] | BatchMiddlewareInterface and AfterwareInterface[] | BatchAfterwareInterface.

[helfer]

@lewisf Awesome, I think this looks much neater, so I think we can merge it soon! Just one question: what's the rationale for renaming the HTTPFetchNetworkInterface to BaseNetworkInterface? I think it would be much better to avoid changes to the external API like this, even if the names are a bit less meaningful.

[lewisf]

@helfer oh, good really good point, I'll change that back.

EDIT: Oh I remember why now, what I labeled as "BaseNetworkInterface" wasn't actually defining the behavior for 'fetch'. Instead, fetchFromRemoteEndpoint was being defined in the impl, which I left as HTTPFetchNetworkInterface

[tomitrescak]

Is there any documentation on what 'applyBatchMiddleware' should contain? thanks.

[edit] Checking the code it should be part of the network interface, maybe I'm doing something wrong as now I'm getting Uncaught Error: Batch afterware must implement the applyBatchAfterware function which is clearly defined

import ApolloClient, { createBatchingNetworkInterface } from 'apollo-client'; 

const networkInterface = createBatchingNetworkInterface({uri: url, batchInterval: 50});

    networkInterface.use([{
      applyBatchMiddleware(req: any, next: any) {
        if (!req.options.headers) {
          req.options.headers = {};  // Create the header object if needed.
        }
        const token = localStorage.getItem('jwtToken');
        if (token) {
          req.options.headers.authorization = token;
        }
        next();
      }
    }]);

    networkInterface.useAfter([{
      applyBatchAfterware(_response: any, next: any) {
        next();
      }

[EDIT] OK. Got it. I had applyAterware in my code. So simply .. it was just renamed.

[advance512]

A documentation update would indeed be very welcome :)

[lewisf]

@advance512 I have a PR open for a documentation update: https://github.com/apollographql/core-docs/pull/256 :)