Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

[proposal] split the package into core + framework interface #178

Closed
mattecapu opened this issue Dec 29, 2016 · 3 comments
Closed

[proposal] split the package into core + framework interface #178

mattecapu opened this issue Dec 29, 2016 · 3 comments

Comments

@mattecapu
Copy link

mattecapu commented Dec 29, 2016

I recently stumbled upon react-relay-network-layer which provides a middleware for query batching. It does all sorts of bad things to fake an HTTP request to this middleware.
I needed to port the same middleware to Koa, and thus I find myself doing the same bad things to achieve the same thing against koa-graphql, the port of this package to Koa.

So there are at least 4 different packages whose job is:

  1. Get an HTTP GraphQL request
  2. Resolve it given some options
  3. Return an HTTP response.

The second point is literally the same for everyone, but we can't use a common solution because request/response interfaces are different from framework to framework (or even from use case to use case, as for query batching).

Thus I propose to create a package (say http-graphql) to provide this functionality through a common, framework-agnostic core to resolve an HTTP GraphQL request.
Then express-graphql package could simply wrap http-graphql with express-specific logic, and the same can do koa-graphql, koa-graphql-batch and anyone who needs to process and HTTP GraphQL request.

It's just a matter of surgically remove express-specific idioms from this package and replace it with the most slim and simple common interface we can come up with (maybe repurpose Node.js own interfaces?).
From a user standpoint, this would be a simple minor version bump to a GraphQL package, but with improved customization, stability and reliability.

Issue #113 could be addressed while creating the new http-graphql package, by exporting both a silver-bullet function resolveHttpGraphQLRequest(req: HttpRequest) : HttpResponse) and more granular functions like parser, validate, execute, ...

(I'd be happy to do this! I studied the codebase and I think I could manage the work. Obviously the new repo would be under the graphql organization.)

@nodkz
Copy link

nodkz commented Dec 30, 2016

Great thoughts according to a freeze #100

Current implementation of graphqlBatchHTTPWrapper.js with fake sub requests and responces. In other words mess of dirty hacks:

export default function (graphqlHTTPMiddleware) {
  return (req, res) => {
    const subResponses = [];
    return Promise.all(
      req.body.map(data =>
        new Promise((resolve) => {
          const subRequest = {
            __proto__: req.__proto__, // eslint-disable-line
            ...req,
            body: data,
          };
          const subResponse = {
            status(st) { this.statusCode = st; return this; },
            set() { return this; },
            send(payload) {
              resolve({ status: this.statusCode, id: data.id, payload });
            },

            // support express-graphql@0.5.2
            setHeader() { return this; },
            header() {},
            write(payload) { this.payload = payload; },
            end(payload) {
              // support express-graphql@0.5.4
              if (payload) {
                this.payload = payload;
              }
              resolve({ status: this.statusCode, id: data.id, payload: this.payload });
            },
          };
          subResponses.push(subResponse);
          graphqlHTTPMiddleware(subRequest, subResponse);
        })
      )
    ).then(responses => {
      let response = '';
      responses.forEach(({ status, id, payload }, idx) => {
        if (status) {
          res.status(status);
        }
        const comma = responses.length - 1 > idx ? ',' : '';
        response += `{ "id":"${id}", "payload":${payload} }${comma}`;
      });
      res.set('Content-Type', 'application/json');
      res.send(`[${response}]`);
    }).catch(err => {
      res.status(500).send({ error: err.message });
    });
  };
}

@mattecapu
Copy link
Author

@leebyron @wincent @chentsulin what do you think?

@wincent
Copy link
Contributor

wincent commented Jan 25, 2017

I think there's clearly a demand for this (as evidenced by this issue, and #113 and #101). In essence, all of these are about the same thing: making express-graphql more extensible and reusable. The actual mechanism we use to do that needs to be decided (whether it be splitting it out into smaller libraries that can be recomposed, or creating a structure that makes it easy for other libraries to insert themselves in some way), but I think there's value in the core, abstract idea independently of any implementation details.

We should centralize the discussion in a single place though, so I am going to close this issue and request that we concentrate further discussion in issue #113.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants