mcansh / remix-fastify

Fastify server request handler for Remix
https://remix-fastify.onrender.com
MIT License
181 stars 25 forks source link

Why is noopContentParser needed? #244

Closed adaboese closed 7 months ago

adaboese commented 7 months ago
const noopContentParser: FastifyContentTypeParser = (
  _request,
  payload,
  done,
) => {
  done(null, payload);
};

app.addContentTypeParser('application/json', noopContentParser);
app.addContentTypeParser('*', noopContentParser);

Just wondering what purpose this serves

mcansh commented 7 months ago

we don't want fasitfy to parse any formData from incoming requests, so we're just passing it through as is. the reason for both json and * being set is because * doesn't handle json for whatever reason

https://github.com/mcansh/remix-fastify/issues/116#issuecomment-1694540545

adaboese commented 7 months ago

Thanks for sharing that context!

adaboese commented 7 months ago

we don't want fasitfy to parse any formData from incoming requests, so we're just passing it through as is. the reason for both json and being set is because doesn't handle json for whatever reason

What if I have a Fastify route before Remix route that requires JSON parsing?

adaboese commented 7 months ago

I've realized after a bunch of debugging that this is causing issues with other Fastify routes, e.g. I am using Inngest, which registers its own Fastify routes and expects the default Fastify behavior for it to function properly.

Is there a way to make remix-fastify work without these overrides?

adaboese commented 7 months ago

Seeing errors like:

{
  "level": 50,
  "time": 1707252794972,
  "pid": 82086,
  "reqId": "req-8",
  "req": {
    "method": "POST",
    "url": "/test-post",
    "hostname": "127.0.0.1:3000",
    "remoteAddress": "127.0.0.1",
    "remotePort": 56949
  },
  "res": {
    "statusCode": 500
  },
  "err": {
    "type": "TypeError",
    "message": "Converting circular structure to JSON\n    --> starting at object with constructor 'Socket'\n    |     property 'parser' -> object with constructor 'HTTPParser'\n    --- property 'socket' closes the circle",
    "stack": "TypeError: Converting circular structure to JSON\n    --> starting at object with constructor 'Socket'\n    |     property 'parser' -> object with constructor 'HTTPParser'\n    --- property 'socket' closes the circle\n    at JSON.stringify (<anonymous>)\n    at Object.<anonymous> (file:///Users/ada/Developer/adaboese/aimd/app/bin/server.ts:1:3291)\n    at preHandlerCallback (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/handleRequest.js:137:37)\n    at next (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/hooks.js:233:9)\n    at Object.<anonymous> (file:///Users/ada/Developer/adaboese/aimd/app/bin/server.ts:1:2459)\n    at hookIterator (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/hooks.js:405:10)\n    at next (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/hooks.js:239:18)\n    at hookRunner (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/hooks.js:261:5)\n    at validationCompleted (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/handleRequest.js:114:5)\n    at preValidationCallback (/Users/ada/Developer/adaboese/aimd/node_modules/.pnpm/fastify@4.26.0/node_modules/fastify/lib/handleRequest.js:98:5)"
  },
  "msg": "Converting circular structure to JSON\n    --> starting at object with constructor 'Socket'\n    |     property 'parser' -> object with constructor 'HTTPParser'\n    --- property 'socket' closes the circle"
}
adaboese commented 7 months ago

Meanwhile, if I try to remove content parsers, and add @fastify/formbody, then Remix is not able to process the output.

adaboese commented 7 months ago

@mcansh Maybe the solution is to make remix-fastify work with https://www.npmjs.com/package/fastify-raw-body ?

mcansh commented 7 months ago

@mcansh Maybe the solution is to make remix-fastify work with npmjs.com/package/fastify-raw-body ?

the server is fully in your control, so you're able to use that already if you'd like.

Remix just needs the raw request body in order to parse it. i'm not sure if fastify has a way to only set the content parser for specific routes or not.

adaboese commented 7 months ago

At the moment I am trying to figure out what exactly is breaking.

Looking at the implementation, it looks like remix-fastify is just taking request.raw and passing it through to Remix.

https://github.com/mcansh/remix-fastify/blob/9300c224caec80742e73c1dc65f7e2bc1b0b44e0/packages/remix-fastify/src/server.ts#L112-L115

I don't see what's the relationship with the Fastify content type parsers.

mcansh commented 7 months ago

without telling fastify to not process the json, it does, and that messes up things down the line. if you want to test out other ways to go about, i'm all for it. in the interim you can parse the json in the request handler that needs it.

// by default fastify parses application/json and text/plain payloads, we want remix to parse all content types
// if you have routes outside of Remix that need to parse JSON, you can parse the json in the route handler
app.addContentTypeParser(
  ["application/json", "text/plain", "*"],
  (_request, payload, done) => {
    done(null, payload);
  },
);

function parseJsonFromRequest(request) {
  return new Promise((resolve, reject) => {
    let body = "";
    request.raw.on("data", (chunk) => {
      body += chunk.toString();
    });

    request.raw.on("end", () => {
      try {
        resolve(JSON.parse(body));
      } catch (error) {
        reject(error);
      }
    });
  });
}

app.post("/api/echo", async (request, reply) => {
  let body = await parseJsonFromRequest(request);
  console.log("echo", body);
  reply.status(200);
  reply.header("Content-Type", "application/json");
  reply.send(body);
});
adaboese commented 7 months ago

I think that the ideal setup (given that this is a plugin for Fastify) is that remix-fastify should expect that parsing is already done at the Fastify level and Remix request is constructed using the parsed request.

adaboese commented 7 months ago

This is what I've tried doing the past hour, but seems like Remix Request is not really standard compliant?

e.g. I would expect this to work:

if (request.method !== "GET" && request.method !== "HEAD") {
  // This naively assumes that every request has form body which is already parsed by `@fastify/formbody`
  const formData = new FormData();

  for (const [key, value] of Object.entries(request.body)) {
    formData.append(key, value);
  }

  init.body = formData;

  // init.body = createReadableStreamFromReadable(request.raw);
  init.duplex = "half";
}

but on Remix side I am still seeing empty request.

mcansh commented 7 months ago

I think that the ideal setup (given that this is a plugin for Fastify) is that remix-fastify should expect that parsing is already done at the Fastify level and Remix request is constructed using the parsed request.

this hasn't been a plugin for quite a while to allow for situations like this where you need more control over the server.

adaboese commented 7 months ago

Okay. I got it to work.

if (request.method !== 'GET' && request.method !== 'HEAD') {
  if (
    headers
      ?.get('content-type')
      ?.includes('application/x-www-form-urlencoded')
  ) {
    const payload = new URLSearchParams();

    // @ts-expect-error - We've already parsed the body using @fastify/formbody
    for (const [key, value] of Object.entries(request.body)) {
      payload.append(key, String(value));
    }

    const body = new URLSearchParams(request.body as string);

    init.body = '?' + body.toString();
  } else {
    init.body = createReadableStreamFromReadable(request.raw);
  }

  (init as { duplex: 'half' }).duplex = 'half';
}

Had to path remix-fastify to make it work, but this allows me to not mess with Fastify content parser, which is preferred.

Just need to do the same for JSON.

adaboese commented 7 months ago

FYI this was the suggestion I ended up going with https://github.com/fastify/fastify/discussions/5306#discussioncomment-8399986

mcansh commented 7 months ago

oh nice! if you want to open a PR to do this for the three examples, i'd be happy to merge 🙂