Proxy readable rawBody on request for Next.js #116
Conversation
|
I have the same problem with Remix and had to patch it like this, but I'm afraid this will not allow streaming responses to work if I try to use them: diff --git a/node_modules/@remix-run/express/dist/server.js b/node_modules/@remix-run/express/dist/server.js
index c827141..8fd2a36 100644
--- a/node_modules/@remix-run/express/dist/server.js
+++ b/node_modules/@remix-run/express/dist/server.js
@@ -82,7 +83,7 @@ function createRemixRequest(req, res) {
signal: controller.signal
};
if (req.method !== "GET" && req.method !== "HEAD") {
- init.body = node.createReadableStreamFromReadable(req);
+ init.body = req.rawBody ?? node.createReadableStreamFromReadable(req);
init.duplex = "half";
}
return new Request(url.href, init);I hope your PR can also be useful for Remix. 🤞 |
|
Unfortunately my suggested patch alone doesn't fix the problem completely. For some reason it's still necessary to patch Next.js like this: diff --git a/node_modules/next/dist/server/base-http/node.js b/node_modules/next/dist/server/base-http/node.js
index a2e6c07..8949856 100644
--- a/node_modules/next/dist/server/base-http/node.js
+++ b/node_modules/next/dist/server/base-http/node.js
@@ -23,6 +23,7 @@ _export(exports, {
const _apiutils = require("../api-utils");
const _requestmeta = require("../request-meta");
const _index = require("./index");
+const { Readable } = require("stream");
let _NEXT_REQUEST_META = _requestmeta.NEXT_REQUEST_META;
class NodeNextRequest extends _index.BaseNextRequest {
get originalRequest() {
@@ -37,7 +38,7 @@ class NodeNextRequest extends _index.BaseNextRequest {
this._req = value;
}
constructor(_req){
- super(_req.method.toUpperCase(), _req.url, _req);
+ super(_req.method.toUpperCase(), _req.url, _req.rawBody instanceof Buffer ? Readable.from(_req.rawBody): _req);
this._req = _req;
this.headers = this._req.headers;
this[_NEXT_REQUEST_META] = this._req[_requestmeta.NEXT_REQUEST_META] || {};Otherwise an exception like this can occur (when submitting a POST form request): Next.js uses some precompiled code in production (presumable for the Vercel edge runtime) so it's quite difficult to figure out how all the code depends on each other. I'm afraid there may not be an easy fix here after all! |
|
@leoortizz would you mind take a peek at this solution? |
|
@mfellner Thanks for the initiative! Unfortunately we can't patch Next.js. Can you share how you were testing this fix? |
|
Hey @leoortizz, sure. Basically, I'm applying 3 patches on my side: Patch for Next.js to use a Readable of rawBody instead of the request itself as the 3rd constructor argument of NodeNextRequest. I couldn't figure out why this is still needed after the main patch in firebase-frameworks, but it is. Perhaps somewhere there is an unpatched request still being forwarded to Next.js? next+14.0.3.patchdiff --git a/node_modules/next/dist/server/base-http/node.js b/node_modules/next/dist/server/base-http/node.js
index a2e6c07..8949856 100644
--- a/node_modules/next/dist/server/base-http/node.js
+++ b/node_modules/next/dist/server/base-http/node.js
@@ -23,6 +23,7 @@ _export(exports, {
const _apiutils = require("../api-utils");
const _requestmeta = require("../request-meta");
const _index = require("./index");
+const { Readable } = require("stream");
let _NEXT_REQUEST_META = _requestmeta.NEXT_REQUEST_META;
class NodeNextRequest extends _index.BaseNextRequest {
get originalRequest() {
@@ -37,7 +38,7 @@ class NodeNextRequest extends _index.BaseNextRequest {
this._req = value;
}
constructor(_req){
- super(_req.method.toUpperCase(), _req.url, _req);
+ super(_req.method.toUpperCase(), _req.url, _req.rawBody instanceof Buffer ? Readable.from(_req.rawBody): _req);
this._req = _req;
this.headers = this._req.headers;
this[_NEXT_REQUEST_META] = this._req[_requestmeta.NEXT_REQUEST_META] || {};The main "hacky" patch for firebase-frameworks to essentially proxy all the Readable methods to the rawBody stream. This allows Next.js to read the request object as a stream, which is what it expects, but pulls all the data from the rawBody instead. firebase-frameworks+0.10.6.patchdiff --git a/node_modules/firebase-frameworks/dist/next.js/index.js b/node_modules/firebase-frameworks/dist/next.js/index.js
index 87a1714..a1e3628 100644
--- a/node_modules/firebase-frameworks/dist/next.js/index.js
+++ b/node_modules/firebase-frameworks/dist/next.js/index.js
@@ -1,6 +1,7 @@
import { parse } from "url";
import { default as next } from "next";
import LRU from "lru-cache";
+import { Readable } from "stream";
const nextAppsLRU = new LRU({
// TODO tune this
max: 3,
@@ -29,5 +30,49 @@ export const handle = async (req, res) => {
}
await nextApp.prepare();
const parsedUrl = parse(url, true);
- nextApp.getRequestHandler()(req, res, parsedUrl);
+ console.error("FIREBASE FRAMEWORKS REQ.BODY", typeof req.body, req.body);
+ console.error("FIREBASE FRAMEWORKS REQ.RAWBODY", typeof req.rawBody, req.rawBody);
+ if (req.rawBody instanceof Buffer) {
+ // Proxy all the Readable methods to the rawBody stream.
+ const rawBodyReadable = Readable.from(req.rawBody);
+ const reqProxy = new Proxy(req, {
+ get(target, prop) {
+ if (
+ prop === "read" ||
+ prop === "write" ||
+ prop === "pipe" ||
+ prop === "on" ||
+ prop === "closed" ||
+ prop === "setHeader" ||
+ prop === "writable" ||
+ prop === "req" ||
+ prop === "destroy" ||
+ prop === "destroyed" ||
+ prop === "push" ||
+ prop === "emit" ||
+ prop === "domain" ||
+ prop === "writableErrored" ||
+ prop === "readableErrored" ||
+ prop === "_read" ||
+ prop === "_events" ||
+ prop === "_eventsCount" ||
+ prop === "_readableState" ||
+ prop === "_readableState" ||
+ prop === "_writableState" ||
+ prop === "_destroy" ||
+ prop === Symbol.asyncIterator ||
+ prop === Symbol.asyncDispose ||
+ (typeof prop === "symbol" &&
+ prop.toString() === "Symbol(nodejs.stream.writable)")
+ ) {
+ return Reflect.get(rawBodyReadable, prop);
+ } else {
+ return Reflect.get(target, prop);
+ }
+ },
+ });
+ nextApp.getRequestHandler()(reqProxy, res, parsedUrl);
+ } else {
+ nextApp.getRequestHandler()(req, res, parsedUrl);
+ }
};Finally another patch for firebase-tools to make it copy the patches folder and run patch-package as a postinstall script. firebase-tools+12.9.1.patchdiff --git a/node_modules/firebase-tools/lib/frameworks/index.js b/node_modules/firebase-tools/lib/frameworks/index.js
index 3e14291..044f7df 100644
--- a/node_modules/firebase-tools/lib/frameworks/index.js
+++ b/node_modules/firebase-tools/lib/frameworks/index.js
@@ -310,7 +310,9 @@ async function prepareFrameworks(purpose, targetNames, context, options, emulato
(0, utils_2.logWarning)(`This integration expects Node version ${(0, utils_1.conjoinOptions)(constants_2.VALID_ENGINES.node, "or")}. You're running version ${constants_2.NODE_VERSION}, problems may be encountered.`);
}
(_k = packageJson.engines).node || (_k.node = engine.toString());
- delete packageJson.scripts;
+ const originalPackageJsonScripts = packageJson.scripts;
+ packageJson.scripts = {};
+ packageJson.scripts.postinstall = originalPackageJsonScripts?.postinstall;
delete packageJson.devDependencies;
const bundledDependencies = packageJson.bundledDependencies || {};
if (Object.keys(bundledDependencies).length) {
@@ -345,6 +347,12 @@ async function prepareFrameworks(purpose, targetNames, context, options, emulato
await (0, promises_1.writeFile)((0, path_1.join)(functionsDist, "package.json"), JSON.stringify(packageJson, null, 2));
await (0, promises_1.copyFile)(getProjectPath("package-lock.json"), (0, path_1.join)(functionsDist, "package-lock.json")).catch(() => {
});
+ const patches = await new Promise((resolve, reject) => glob(getProjectPath("patches/*"), (err, matches) => {
+ if (err) reject(err);
+ else resolve(matches);
+ }));
+ await (0, fs_extra_1.mkdirp)((0, path_1.join)(functionsDist, "patches"));
+ await Promise.all(patches.map((path) => (0, promises_1.copyFile)(path, (0, path_1.join)(functionsDist, "patches", (0, path_1.basename)(path)))));
if (await (0, fs_extra_1.pathExists)(getProjectPath(".npmrc"))) {
await (0, promises_1.copyFile)(getProjectPath(".npmrc"), (0, path_1.join)(functionsDist, ".npmrc"));
}While this setup works well enough for me, I don't think it's a good long-term solution and probably not suitable for production. Given that the Next.js patch is still needed, I also don't know how to make it all work by only changing firebase-frameworks. However, this approach still demonstrates the underlying issue and how to fix it in principle. Next.js wants to read the request body as a stream from the request object but fails to do so from the Cloud Functions request object. Perhaps the are some clues in the Open Next project which tries to make Next.js work with AWS Lambda. I haven't looked into it yet. As previously discussed on the issue tacker, there are often use cases where it's necessary to have access to the original, unmodified request object. This is especially true for frameworks like Next.js which expect to be able to read the request body as a stream. Maybe there is also some better approach of creating a new request wrapper class which can be used as an adapter for the Next.js server. Such a solution would probably be more robust and easier to maintain than the current patching approach. In terms of testing, I was simply deploying a basic Next.js app with server actions. The POST request of the server action simply times out without the patch. |
|
Closing in favor of #122, thanks for finding the root cause. |
As identified in #116 the root cause of the problems with non-get requests in NextJS on Firebase Hosting is that the ExpressJS' readable has been spent on the body-parser middleware. Solution is to create a new Readable from rawBody and pass that to the NextJS request handler. * Proxy express requests through a new IncomingMessage * Drop the LRU cache of next apps, no longer needed --------- Co-authored-by: Leonardo Ortiz <[email protected]> Co-authored-by: Maximilian Fellner <[email protected]>
First of all, thanks for supporting Next.js on Firebase at all. It's great!
For Server Action POST requests and other features that depend on the request body, Next.js needs to consume the raw request as a readable stream.
However, this is not possible in Cloud Functions because the stream was already consumed and the request payload stored in the
bodyandrawBodyproperties (afaik). The Next.js logic and code (especially for server actions) is so convoluted that there's no real hope of getting it to support the rawBody property.On the other hand, people have asked for a way to turn off body parsing in Cloud Functions since at least 2017 and so far the only outcome has been the
rawBodyproperty.As a workaround we can proxy all the relevant Readable methods to a Readable instance of the rawBody instead. This seems to work but is obviously a major hack.
Anyway, I was hoping to kick off some discussion around this topic with my pull request! 😃
(relates to firebase/firebase-tools#6468)