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

Fix cloudflare env #514

Merged
merged 7 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/weak-hotels-thank.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@opennextjs/aws": patch
---

Fix cloudflare env
Fix an issue with cookies and the node wrapper
Fix some issue with cookies being not properly set when set both in the routing layer and the route itself
Added option for headers priority
2 changes: 2 additions & 0 deletions packages/open-next/src/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -754,12 +754,14 @@ async function createMiddleware() {
overrides: config.middleware?.override,
defaultConverter: "aws-cloudfront",
includeCache: config.dangerous?.enableCacheInterception,
additionalExternals: config.edgeExternals,
});
} else {
await buildEdgeBundle({
entrypoint: path.join(__dirname, "core", "edgeFunctionHandler.js"),
outfile: path.join(outputDir, ".build", "middleware.mjs"),
...commonMiddlewareOptions,
onlyBuildOnce: true,
});
}
}
2 changes: 1 addition & 1 deletion packages/open-next/src/build/createServerBundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export async function createServerBundle(
const routes = fnOptions.routes;
routes.forEach((route) => foundRoutes.add(route));
if (fnOptions.runtime === "edge") {
await generateEdgeBundle(name, options, fnOptions);
await generateEdgeBundle(name, config, options, fnOptions);
} else {
await generateBundle(name, config, options, fnOptions);
}
Expand Down
38 changes: 36 additions & 2 deletions packages/open-next/src/build/edge/createEdgeBundle.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { mkdirSync } from "node:fs";
import url from "node:url";

import { build } from "esbuild";
import fs from "fs";
import path from "path";
import { MiddlewareInfo, MiddlewareManifest } from "types/next-types";
import {
IncludedConverter,
OpenNextConfig,
OverrideOptions,
RouteTemplate,
SplittedFunctionOptions,
Expand All @@ -29,6 +31,8 @@ interface BuildEdgeBundleOptions {
defaultConverter?: IncludedConverter;
additionalInject?: string;
includeCache?: boolean;
additionalExternals?: string[];
onlyBuildOnce?: boolean;
}

export async function buildEdgeBundle({
Expand All @@ -41,7 +45,13 @@ export async function buildEdgeBundle({
overrides,
additionalInject,
includeCache,
additionalExternals,
onlyBuildOnce,
}: BuildEdgeBundleOptions) {
const isInCloudfare =
typeof overrides?.wrapper === "string"
? overrides.wrapper === "cloudflare"
: (await overrides?.wrapper?.())?.edgeRuntime;
await esbuildAsync(
{
entryPoints: [entrypoint],
Expand Down Expand Up @@ -93,7 +103,7 @@ export async function buildEdgeBundle({
"../../core",
"edgeFunctionHandler.js",
),
isInCloudfare: overrides?.wrapper === "cloudflare",
isInCloudfare,
}),
],
treeShaking: true,
Expand All @@ -106,8 +116,13 @@ export async function buildEdgeBundle({
mainFields: ["module", "main"],
banner: {
js: `
import {Buffer} from "node:buffer";
globalThis.Buffer = Buffer;

import {AsyncLocalStorage} from "node:async_hooks";
globalThis.AsyncLocalStorage = AsyncLocalStorage;
${
overrides?.wrapper === "cloudflare"
isInCloudfare
? ""
: `
const require = (await import("node:module")).createRequire(import.meta.url);
Expand All @@ -129,12 +144,30 @@ export async function buildEdgeBundle({
},
options,
);

if (!onlyBuildOnce) {
await build({
entryPoints: [outfile],
outfile,
allowOverwrite: true,
bundle: true,
minify: true,
platform: "node",
format: "esm",
conditions: ["workerd", "worker", "browser"],
external: ["node:*", ...(additionalExternals ?? [])],
banner: {
js: 'import * as process from "node:process";',
},
});
}
}

export function copyMiddlewareAssetsAndWasm({}) {}

export async function generateEdgeBundle(
name: string,
config: OpenNextConfig,
options: BuildOptions,
fnOptions: SplittedFunctionOptions,
) {
Expand Down Expand Up @@ -193,5 +226,6 @@ export async function generateEdgeBundle(
outfile: path.join(outputPath, "index.mjs"),
options,
overrides: fnOptions.override,
additionalExternals: config.edgeExternals,
});
}
1 change: 1 addition & 0 deletions packages/open-next/src/core/createMainHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ declare global {
requestId: string;
pendingPromiseRunner: DetachedPromiseRunner;
isISRRevalidation?: boolean;
mergeHeadersPriority?: "middleware" | "handler";
}>;
}

Expand Down
14 changes: 13 additions & 1 deletion packages/open-next/src/core/requestHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ globalThis.__als = new AsyncLocalStorage<{
requestId: string;
pendingPromiseRunner: DetachedPromiseRunner;
isISRRevalidation?: boolean;
mergeHeadersPriority?: "middleware" | "handler";
}>();

patchAsyncStorage();
Expand Down Expand Up @@ -103,8 +104,19 @@ export async function openNextHandler(
const pendingPromiseRunner: DetachedPromiseRunner =
new DetachedPromiseRunner();
const isISRRevalidation = headers["x-isr"] === "1";
const mergeHeadersPriority = globalThis.openNextConfig.dangerous
?.headersAndCookiesPriority
? globalThis.openNextConfig.dangerous.headersAndCookiesPriority(
preprocessedEvent,
)
: "middleware";
const internalResult = await globalThis.__als.run(
{ requestId, pendingPromiseRunner, isISRRevalidation },
{
requestId,
pendingPromiseRunner,
isISRRevalidation,
mergeHeadersPriority,
},
async () => {
const preprocessedResult = preprocessResult as MiddlewareOutputEvent;
const req = new IncomingMessage(reqProps);
Expand Down
48 changes: 29 additions & 19 deletions packages/open-next/src/http/openNextResponse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,6 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
private initialHeaders?: OutgoingHttpHeaders,
) {
super();
if (initialHeaders && initialHeaders[SET_COOKIE_HEADER]) {
this._cookies = parseCookies(
initialHeaders[SET_COOKIE_HEADER] as string | string[],
) as string[];
}
this.once("finish", () => {
if (!this.headersSent) {
this.flushHeaders();
Expand Down Expand Up @@ -164,28 +159,43 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
this.headersSent = true;
// Initial headers should be merged with the new headers
// These initial headers are the one created either in the middleware or in next.config.js
// We choose to override response headers with middleware headers
// This is different than the default behavior in next.js, but it allows more customization
// TODO: We probably want to change this behavior in the future to follow next
// We could add a prefix header that would allow to force the middleware headers
// Something like open-next-force-cache-control would override the cache-control header
const mergeHeadersPriority =
globalThis.__als?.getStore()?.mergeHeadersPriority ?? "middleware";
if (this.initialHeaders) {
this.headers = {
...this.headers,
...this.initialHeaders,
};
this.headers =
mergeHeadersPriority === "middleware"
? {
...this.headers,
...this.initialHeaders,
}
: {
...this.initialHeaders,
...this.headers,
};
const initialCookies = parseCookies(
(this.initialHeaders[SET_COOKIE_HEADER] as string | string[]) ?? [],
) as string[];
this._cookies =
mergeHeadersPriority === "middleware"
? [...this._cookies, ...initialCookies]
: [...initialCookies, ...this._cookies];
}
this.fixHeaders(this.headers);
if (this._cookies.length > 0) {
// For cookies we cannot do the same as for other headers
this.headers[SET_COOKIE_HEADER] = this._cookies;
}

// We need to fix the set-cookie header here
this.headers[SET_COOKIE_HEADER] = this._cookies;

const parsedHeaders = parseHeaders(this.headers);

// We need to remove the set-cookie header from the parsed headers because
// it does not handle multiple set-cookie headers properly
delete parsedHeaders[SET_COOKIE_HEADER];

if (this.streamCreator) {
this.responseStream = this.streamCreator?.writeHeaders({
statusCode: this.statusCode ?? 200,
cookies: this._cookies,
headers: parseHeaders(this.headers),
headers: parsedHeaders,
});
this.pipe(this.responseStream);
}
Expand Down
8 changes: 0 additions & 8 deletions packages/open-next/src/plugins/edge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,8 @@ export function openNextEdgePlugins({
contents = `
globalThis._ENTRIES = {};
globalThis.self = globalThis;
if(!globalThis.process){
globalThis.process = {env: {}};
}
globalThis._ROUTES = ${JSON.stringify(routes)};
import {Buffer} from "node:buffer";
globalThis.Buffer = Buffer;
import {AsyncLocalStorage} from "node:async_hooks";
globalThis.AsyncLocalStorage = AsyncLocalStorage;
${
isInCloudfare
? ``
Expand Down
9 changes: 9 additions & 0 deletions packages/open-next/src/types/open-next.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ export interface DangerousOptions {
* @default false
*/
enableCacheInterception?: boolean;
/**
* Function to determine which headers or cookies takes precedence.
* By default, the middleware headers and cookies will override the handler headers and cookies.
* This is executed for every request and after next config headers and middleware has executed.
*/
headersAndCookiesPriority?: (
event: InternalEvent,
) => "middleware" | "handler";
}

export type BaseOverride = {
Expand Down Expand Up @@ -83,6 +91,7 @@ export type Wrapper<
> = BaseOverride & {
wrapper: WrapperHandler<E, R>;
supportStreaming: boolean;
edgeRuntime?: boolean;
};

export type Warmer = BaseOverride & {
Expand Down
6 changes: 1 addition & 5 deletions packages/open-next/src/wrappers/aws-lambda-streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ const handler: WrapperHandler = async (handler, converter) =>
return;
}

let headersWritten = false;

const internalEvent = await converter.convertFrom(event);

//Handle compression
Expand Down Expand Up @@ -84,14 +82,12 @@ const handler: WrapperHandler = async (handler, converter) =>
"application/vnd.awslambda.http-integration-response",
);
_prelude.headers["content-encoding"] = contentEncoding;
// We need to remove the set-cookie header as otherwise it will be set twice, once with the cookies in the prelude, and a second time with the set-cookie headers
delete _prelude.headers["set-cookie"];

const prelude = JSON.stringify(_prelude);

responseStream.write(prelude);

responseStream.write(new Uint8Array(8));
headersWritten = true;

return compressedStream ?? responseStream;
},
Expand Down
13 changes: 11 additions & 2 deletions packages/open-next/src/wrappers/cloudflare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,16 @@ const handler: WrapperHandler<
> =
async (handler, converter) =>
async (event: Request, env: Record<string, string>): Promise<Response> => {
//@ts-expect-error - process is not defined in cloudflare workers
globalThis.process = { env };
globalThis.process = process;
Copy link
Collaborator

@khuezy khuezy Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the process variable coming from? Is that specific to cloudflare?
Oh nvm, it's the global.process... got confused w/ the globalThis

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this as a js banner in the esbuild that i did for the cloudflare worker.
Now that i think about it we should probably do this esbuild part inside of OpenNext as well. We'd get a single file out of it which is easier for cloudflare worker.

I'll publish a sample repo later with SST v3 with different way to deploy OpenNext on different target.
Right now i have EC2, ECS and lambda with either cloudfront or cloudflare in front of it ( and with cloudflare the middleware runs in the worker )
Maybe that's something we could put in the new org as well


// Set the environment variables
// Cloudlare suggests to not override the process.env object but instead apply the values to it
for (const [key, value] of Object.entries(env)) {
if (typeof value === "string") {
process.env[key] = value;
}
}

const internalEvent = await converter.convertFrom(event);

const response = await handler(internalEvent);
Expand All @@ -27,4 +35,5 @@ export default {
wrapper: handler,
name: "cloudflare",
supportStreaming: true,
edgeRuntime: true,
};
2 changes: 2 additions & 0 deletions packages/open-next/src/wrappers/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ const wrapper: WrapperHandler = async (handler, converter) => {
const internalEvent = await converter.convertFrom(req);
const _res: StreamCreator = {
writeHeaders: (prelude) => {
res.setHeader("Set-Cookie", prelude.cookies);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Set-Cookie allows for setting multiple cookies at once (could be wrong). Need to split the cookies and res.addHeader if so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it does ( I thought the same thing as well at first ).

Copy link
Collaborator

@khuezy khuezy Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really weird, according to the spec: https://httpwg.org/specs/rfc6265.html#overview it shouldn't. Since the comma character may also be part of the cookie value, eg Expires=Wed, 09 Jun 2021 10:18:14 GMT

I wonder what the prelude.cookies is, is it an array? Does it get parsed out and handle somewhere else in the stack, or does res.setHeader here ultimately go back to the client.

Edit: I tried doing comma separated Set-Cookies in my server and it doesn't work. Only the first cookie is set. But doing multiple Set-Cookie for each cookie works as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@sommeeeer sommeeeer Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I missed that the node response setHeader is doing the heavy lifting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there was an issue here ( i forgot to remove the custom wrapper on my test 🙄 ).
We needed to remove the set-cookie header from the headers, otherwise the broken one would take precedence.
There was also another issue with the set-cookie that affected everything. If cookies were set both in the middleware and in a route, only the middleware ones would be applied

res.writeHead(prelude.statusCode, prelude.headers);
res.flushHeaders();
res.uncork();
return res;
},
Expand Down
Loading