Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage with fastify middleware #75

Open
taneba opened this issue Oct 10, 2021 · 6 comments
Open

Usage with fastify middleware #75

taneba opened this issue Oct 10, 2021 · 6 comments
Labels
help wanted Extra attention is needed

Comments

@taneba
Copy link
Contributor

taneba commented Oct 10, 2021

Is there a way to make sendResult work with fastify middleware?

for example:

const app = fastify()
app.register(fastifyCors)
app.route({
  method: ['GET', 'POST'],
  url: '/graphql',
  hanlder: async (req,res) => {
    //...
    const result = processRequest({...})
    sendRequest(request, res.raw)
  }
})

In the code above, I want fastifyCors to add access-control-allow-origin header to the response, but it won't.
I'm not completely sure but is that because sendResult is handling "RawResponse" rather than "Response" which is extended by fastify?

@dotansimha
Copy link
Collaborator

Hi @taneba ,
Thanks for reaching out. Currently, sendResult is using the raw request because we wanted it to be agnostic to the HTTP server.
I think in cases where you want to use different setup, you might need to use the code from sendResult and adjust to to your needs (and use fastify and not the raw HTTP).

I'm not sure how to address this, because it seems specific for fastify?

@n1ru4l thoughts?

@dotansimha dotansimha added the help wanted Extra attention is needed label Oct 28, 2021
@taneba
Copy link
Contributor Author

taneba commented Nov 2, 2021

@dotansimha
Thank you for elaborating that and it makes sense to keep it framework agnostic.

How about taking an approach like what graphql-ws is doing, which is to have a helper function for certain library.

Their example for fastify:

import Fastify from 'fastify'; // yarn add fastify
import fastifyWebsocket from 'fastify-websocket'; // yarn add fastify-websocket
import { makeHandler } from 'graphql-ws/lib/use/fastify-websocket';
import { schema } from './previous-step';

const fastify = Fastify();
fastify.register(fastifyWebsocket);

fastify.get('/graphql', { websocket: true }, makeHandler({ schema }));

fastify.listen(4000, (err) => {
  if (err) {
    fastify.log.error(err);
    return process.exit(1);
  }
  console.log('Listening to port 4000');
});

@dotansimha
Copy link
Collaborator

Just to mention that we had a similar issue today, with fastify-cors. The headers set by this plugin was added to the fastify.reply and not to fastify.reply.raw, and sendNodeResult was ignoring those.

Doing that before calling sendNodeResult should work as a temporary workaround:

for (const [key, value] of Object.entries(res.getHeaders())) {
  res.raw.setHeader(key, String(value || ''))
}

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 23, 2021

I checked out how different plugins handle sending responses with res.raw.write:

https://github.com/NodeFactoryIo/fastify-sse-v2/blob/b1686a979fbf655fb9936c0560294a0c094734d4/src/plugin.ts#L6-L21

export const plugin: FastifyPluginAsync<SsePluginOptions> = async function(
  instance,
  options
): Promise<void> {
  instance.decorateReply("sse", function(
    this: FastifyReply,
    source: AsyncIterable<EventMessage>
  ): void {
    Object.entries(this.getHeaders()).forEach(([key, value]) => {
      this.raw.setHeader(key, value);
    });
    this.raw.setHeader("Content-Type", "text/event-stream");
    this.raw.setHeader("Connection", "keep-alive");
    this.raw.setHeader("Cache-Control", "no-cache,no-transform");
    this.raw.setHeader("x-no-compression", 1);
    this.raw.write(serializeSSEEvent({ retry: options.retryDelay || 3000 }));
    toStream(transformAsyncIterable(source)).pipe(this.raw);
  });
};

The solution you proposed @dotansimha seems to be the correct one!


We should probably only give users the headers and the payload. The responsibility for glueing that to the framework of their choice should be more or less straightforward and can be guided with tutorials, recipes and example applications.

One of the main reasons why we added these helpers was boilerplate code we had to write over and over again for the multi-part protocol. Now we realize that we tried to own too much...

Instead of owning the whole pipeline we could only have helper functions for formatting the payload into the desired protocol and yield the string partials to the user:

async function* multiPartProtocol(
  source: AsyncIterable<ExecutionResult>,
  transformResult: TransformResultFn
) {
  yield "---";
  for await (const result of source) {
    const chunk = Buffer.from(JSON.stringify(transformResult(result)), "utf8");
    const data = [
      "",
      "Content-Type: application/json; charset=utf-8",
      "Content-Length: " + String(chunk.length),
      "",
      chunk
    ];

    if (result.hasNext) {
      data.push("---");
    }
    yield "\r\n";
  }
  yield "\r\n-----\r\n";
}
async function* sseProtocol(
  source: AsyncIterable<ExecutionResult>,
  transformResult: TransformResultFn
) {
  for await (const result of source) {
    yield `data: ${JSON.stringify(transformResult(result))}\n\n`;
  }
}

Furthermore, I spotted that Fastify can handle stream payloads 🤔 https://github.com/fastify/fastify/blob/0439d5ffcab1709659353e8633a9a1ff36595c22/lib/reply.js#L426-L431

SO maybe the result returned from processRequest, could implement that interface (if it is a stream response and not only a single payload). Then for fastify we actually might not need to use res.raw at all!

From a user-land perspective the code would look like this:

app.route({
  method: ["GET", "POST"],
  url: "/graphql",
  async handler(req, res) {
    const request = {
      body: req.body,
      headers: req.headers,
      method: req.method,
      query: req.query,
    };

    if (shouldRenderGraphiQL(request)) {
      res.type("text/html");
      res.send(renderGraphiQL({}));
    } else {
      const request = {
        body: req.body,
        headers: req.headers,
        method: req.method,
        query: req.query,
      };
      const { operationName, query, variables } = getGraphQLParameters(request);
      const { headers, source } = await processRequest({
        operationName,
        query,
        variables,
        request,
        schema,
      });

      // headers determined by helix
      for (const header of headers) {
        res.setHeader(header.key, header.value)
      }
      // source is a stream or string
      res.send(source)
    }
  },
});

There is still an open question for me: Should we even tell people what headers/protocols they should use? #161

@ricardo-valero
Copy link

I'm a bit biased because it's what I'm using right now, but check out graphql-ez it uses graphql-helix and has several integrations, not only for fastify, but express, koa and more.

Here's the fastify integration

Hope it helps in some way!

@jetaggart
Copy link

Any further thoughts on this? This was incredible confusing to figure out. I'm using fastify-cors and this took hours to figure out that the helix setup was swallowing the headers. @dotansimha answer worked for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

No branches or pull requests

5 participants