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.

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

Jump to bottom

templates(cloudflare-workers): use static assets #10034

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+44 −55
Open

templates(cloudflare-workers): use static assets #10034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10776e5
update cloudflare worker template to use static assets
edmundhung Sep 26, 2024
1b52fe4
pin wrangler version
edmundhung Sep 27, 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
4 changes: 0 additions & 4 deletions templates/cloudflare-workers/env.d.ts
View file
Open in desktop

This file was deleted.

22 changes: 15 additions & 7 deletions templates/cloudflare-workers/load-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import { type PlatformProxy } from "wrangler";

// PlatformProxy’s caches property is incompatible with the caches global
// https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/api/integrations/platform/caches.ts
type Cloudflare = Omit<PlatformProxy<Env>, "dispose" | "caches"> & {
caches: CacheStorage;
};
type GetLoadContextArgs = {
request: Request;
context: {
cloudflare: Omit<PlatformProxy<Env, IncomingRequestCfProperties>, "dispose" | "caches"> & {
caches: PlatformProxy<Env, IncomingRequestCfProperties>['caches'] | CacheStorage;
};
};
}

declare module "@remix-run/cloudflare" {
interface AppLoadContext {
cloudflare: Cloudflare;
// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface AppLoadContext extends ReturnType<typeof getLoadContext> {
// This will merge the result of `getLoadContext` into the `AppLoadContext`
}
}

export function getLoadContext({ context }: GetLoadContextArgs) {
return context;
}
Comment on lines +19 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function getLoadContext({ context }: GetLoadContextArgs) {
return context;
}
export const getLoadContext = ({ context }: GetLoadContextArgs) => context;

5 changes: 2 additions & 3 deletions templates/cloudflare-workers/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"typecheck": "tsc"
},
"dependencies": {
"@cloudflare/kv-asset-handler": "^0.3.2",
"@remix-run/cloudflare": "*",
"@remix-run/react": "*",
"@remix-run/server-runtime": "*",
Expand All @@ -21,7 +20,7 @@
"react-dom": "^18.2.0"
},
"devDependencies": {
"@cloudflare/workers-types": "^4.20240512.0",
"@cloudflare/workers-types": "^4.20240925.0",
"@remix-run/dev": "*",
"@types/react": "^18.2.20",
"@types/react-dom": "^18.2.7",
Expand All @@ -39,7 +38,7 @@
"typescript": "^5.1.6",
"vite": "^5.1.0",
"vite-tsconfig-paths": "^4.2.1",
"wrangler": "3.57.1"
"wrangler": "3.78.10"
},
"engines": {
"node": ">=20.0.0"
Expand Down
55 changes: 18 additions & 37 deletions templates/cloudflare-workers/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,56 +1,37 @@
import { getAssetFromKV } from "@cloudflare/kv-asset-handler";
import { createRequestHandler, type ServerBuild } from "@remix-run/cloudflare";
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore This file won’t exist if it hasn’t yet been built
import * as build from "./build/server"; // eslint-disable-line import/no-unresolved
// eslint-disable-next-line import/no-unresolved
import __STATIC_CONTENT_MANIFEST from "__STATIC_CONTENT_MANIFEST";
import { getLoadContext } from "./load-context";

const MANIFEST = JSON.parse(__STATIC_CONTENT_MANIFEST);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const handleRemixRequest = createRequestHandler(build as any as ServerBuild);

export default {
async fetch(request, env, ctx) {
const waitUntil = ctx.waitUntil.bind(ctx);
const passThroughOnException = ctx.passThroughOnException.bind(ctx);
try {
const url = new URL(request.url);
const ttl = url.pathname.startsWith("/assets/")
? 60 * 60 * 24 * 365 // 1 year
: 60 * 5; // 5 minutes
return await getAssetFromKV(
{ request, waitUntil },
{
ASSET_NAMESPACE: env.__STATIC_CONTENT,
ASSET_MANIFEST: MANIFEST,
cacheControl: {
browserTTL: ttl,
edgeTTL: ttl,
const loadContext = getLoadContext({
request,
context: {
cloudflare: {
// This object matches the return value from Wrangler's
// `getPlatformProxy` used during development via Remix's
// `cloudflareDevProxyVitePlugin`:
// https://developers.cloudflare.com/workers/wrangler/api/#getplatformproxy
cf: request.cf,
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes a type error, should it just be cast to any?

Copy link

Choose a reason for hiding this comment

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

I think we can just set it to

cf: request.cf!

Or, we can change load-context.ts to:

type GetLoadContextArgs = {
  request: Request;
  context: {
    cloudflare: Omit<PlatformProxy<Env, IncomingRequestCfProperties>, "dispose" | "caches" | "cf"> & {
      caches: PlatformProxy<Env, IncomingRequestCfProperties>['caches'] | CacheStorage;
      cf: Request['cf'];
    };
  };
}

The first is simpler, but the second is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer correctness, I'll defer to @edmundhung

ctx: {
waitUntil: ctx.waitUntil.bind(ctx),
passThroughOnException: ctx.passThroughOnException.bind(ctx),
},
caches,
env,
},
}
);
} catch (error) {
// No-op
}

try {
const loadContext = {
cloudflare: {
// This object matches the return value from Wrangler's
// `getPlatformProxy` used during development via Remix's
// `cloudflareDevProxyVitePlugin`:
// https://developers.cloudflare.com/workers/wrangler/api/#getplatformproxy
cf: request.cf,
ctx: { waitUntil, passThroughOnException },
caches,
env,
},
};
});
return await handleRemixRequest(request, loadContext);
} catch (error) {
console.log(error);
return new Response("An unexpected error occurred", { status: 500 });
}
},
} satisfies ExportedHandler<Env & { __STATIC_CONTENT: KVNamespace<string> }>;
} satisfies ExportedHandler<Env>;
5 changes: 4 additions & 1 deletion templates/cloudflare-workers/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ import {
cloudflareDevProxyVitePlugin,
} from "@remix-run/dev";
import tsconfigPaths from "vite-tsconfig-paths";
import { getLoadContext } from "./load-context";

export default defineConfig({
plugins: [
cloudflareDevProxyVitePlugin(),
cloudflareDevProxyVitePlugin({
getLoadContext,
}),
remix({
future: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@edmundhung let's go ahead and add

v3_lazyRouteDiscovery: true,
v3_singleFetch: true,

to the config

v3_fetcherPersist: true,
Expand Down
8 changes: 5 additions & 3 deletions templates/cloudflare-workers/wrangler.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
#:schema node_modules/wrangler/config-schema.json
Copy link

Choose a reason for hiding this comment

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

Where's this coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you use an IDE extension that support json schema over TOML, this will give you auto completion and validation on your wrangler.toml, like this one.

Copy link

@moishinetzer moishinetzer Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fabulous - perhaps add it as a comment?

Copy link

Choose a reason for hiding this comment

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

It's generated by the wrangler.

Where's this coming from?

name = "remix-cloudflare-workers-template"

main = "./server.ts"
workers_dev = true
# https://developers.cloudflare.com/workers/platform/compatibility-dates
compatibility_date = "2023-04-20"
compatibility_date = "2024-09-26"

[site]
bucket = "./build/client"
[assets]
# https://developers.cloudflare.com/workers/static-assets/binding/
directory = "./build/client"
edmundhung marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Should we add an ASSETS binding here too?

Copy link
Contributor

@acusti acusti Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@petebacondarwin for the majority of use cases, there’s no need for the binding. the static assets feature intercepts requests and checks if the URL matches anything in the assets directory (source). if it finds a file, it serves that. if not it, invokes your worker. as a result, the ASSETS binding’s only purpose is to provide programmatic access from your worker to the available assets that are already being automatically served when appropriate. i think that’s why the create-cloudflare remix template doesn’t default to including the binding: https://github.com/cloudflare/workers-sdk/blob/main/packages/create-cloudflare/templates-experimental/remix/templates/wrangler.toml


[build]
command = "npm run build"