Skip to content

Fix cloudflare env #514

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Fix cloudflare env #514

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d97633d
fix cookies improperly set on node
conico974 Sep 7, 2024
08fa891
fix issue with env in cloudflare
conico974 Sep 12, 2024
4a47d5a
fix cookies not properly set when set both in the middleware and a route
conico974 Sep 13, 2024
69c71d3
bundle everything for edge into one file
conico974 Sep 13, 2024
7c2277a
avoid bundling in one file when middleware is inside server function
conico974 Sep 13, 2024
88a8675
added option for headers priority
conico974 Sep 17, 2024
b8197ce
Create weak-hotels-thank.md
conico974 Oct 3, 2024
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
2 changes: 2 additions & 0 deletions packages/open-next/src/build.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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,
});
}
26 changes: 16 additions & 10 deletions packages/open-next/src/http/openNextResponse.ts
View file
Open in desktop
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 @@ -174,18 +169,29 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
...this.headers,
...this.initialHeaders,
};
const initialCookies = parseCookies(
(this.initialHeaders[SET_COOKIE_HEADER] as string | string[]) ?? [],
) as string[];
//Do we want to filter out the cookies that are already set?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//Do we want to filter out the cookies that are already set?
// Do we want to filter out the cookies that are already set?

// At the moment cookies from the middlewware will override the ones set in the route
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// At the moment cookies from the middlewware will override the ones set in the route
// At the moment cookies from the middleware will override the ones set in the route

this._cookies = [...this._cookies, ...initialCookies];
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, we should ask the community? In terms of flow, middleware comes before the route so I'd assume the route should have the final say in the cookies. Just my pov, but you could argue the other way depending on your use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could ask the community.
Right now for headers, middleware will override route, it makes sense that it's the same for cookies. The thing with the route having the final say is that in some case ( app router for example ) there is no way to modify the headers others than by having the middleware take precedence.

There is also another option here, we could use an env variable (or an option in the config) to decide which should take priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we could ask the community. Right now for headers, middleware will override route, it makes sense that it's the same for cookies. The thing with the route having the final say is that in some case ( app router for example ) there is no way to modify the headers others than by having the middleware take precedence.

There is also another option here, we could use an env variable (or an option in the config) to decide which should take priority.

I like this idea, especially in the config. However, I think that the middleware should override both cookies and headers by default

}
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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions packages/open-next/src/types/open-next.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,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
View file
Open in desktop
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
View file
Open in desktop
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
Contributor

@khuezy khuezy Sep 12, 2024 •
edited
Loading

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
Contributor 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
View file
Open in desktop
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
Contributor

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
Contributor 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
Contributor

@khuezy khuezy Sep 12, 2024 •
edited
Loading

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
Contributor Author

Choose a reason for hiding this comment

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

prelude.cookies is an array, it's from here
https://github.com/sst/open-next/blob/faefb33a2a485c571c2cc43680f1b702a03b58c7/packages/open-next/src/http/openNextResponse.ts#L187

Copy link
Contributor

@sommeeeer sommeeeer Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It will work with an array: https://nodejs.org/api/http.html#http_response_setheader_name_value

Copy link
Contributor

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
Contributor 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