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 2 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
13 changes: 11 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
Expand Up @@ -42,6 +42,10 @@ export async function buildEdgeBundle({
additionalInject,
includeCache,
}: BuildEdgeBundleOptions) {
const isInCloudfare =
typeof overrides?.wrapper === "string"
? overrides.wrapper === "cloudflare"
: (await overrides?.wrapper?.())?.edgeRuntime;
await esbuildAsync(
{
entryPoints: [entrypoint],
Expand Down Expand Up @@ -93,7 +97,7 @@ export async function buildEdgeBundle({
"../../core",
"edgeFunctionHandler.js",
),
isInCloudfare: overrides?.wrapper === "cloudflare",
isInCloudfare,
}),
],
treeShaking: true,
Expand All @@ -106,8 +110,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 Down
4 changes: 0 additions & 4 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 @@ -176,10 +176,6 @@ export class OpenNextNodeResponse extends Transform implements ServerResponse {
};
}
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;
}

if (this.streamCreator) {
this.responseStream = this.streamCreator?.writeHeaders({
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,
};
1 change: 1 addition & 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,6 +10,7 @@ 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.uncork();
return res;
Expand Down
Loading