-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
templates(cloudflare-workers): use static assets #10034
Conversation
|
0286b11
to
38cd5c9
Compare
c59ad09
to
10776e5
Compare
@edmundhung while you're on it, shouldn't https://github.com/edmundhung/remix/blob/1b52fe4b118735692b94d5338fca39c1b8bc55ae/templates/cloudflare-workers/app/root.tsx#L8 and https://github.com/edmundhung/remix/blob/workers-assets/templates/cloudflare-workers/app/routes/_index.tsx be from |
I've been testing this template and it's been working flawlessly. However, has anyone else managed to make the asset binding work? I keep getting This only occurs when running the |
@ayuhito are you when running So it's showing the page, but I'm seeing the error in the terminal.
|
And this too, they are from the old commits: |
Nope. I've got a different error to that. I was alluding to the fact that |
Is there any rough timeline for when this PR might land? (excited about using it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. If we can land this we can fix up the create-cloudflare tool to point to it.
Does this same setup work for SPA mode? |
Static Assets (aka Workers Assets) is currently in beta, but according to the Wrangler configuration docs, Workers Sites has been deprecated in favor of Workers Assets:
also, i’ve been using it in production (same as the implementation introduced here) without issue for a few weeks now. my only comment on this PR is that i’m not sure whether or not it’s worth including the custom @focux i haven’t tried it, but i can’t imagine why it wouldn’t work for SPA mode. the static assets feature matches any request to any file in the specified @FraBle you can use this right away for new projects by using the fork:
or just manually apply the diff of this PR to an existing Remix / Cloudflare Workers project if you have one. |
export function getLoadContext({ context }: GetLoadContextArgs) { | ||
return context; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function getLoadContext({ context }: GetLoadContextArgs) { | |
return context; | |
} | |
export const getLoadContext = ({ context }: GetLoadContextArgs) => context; |
// `getPlatformProxy` used during development via Remix's | ||
// `cloudflareDevProxyVitePlugin`: | ||
// https://developers.cloudflare.com/workers/wrangler/api/#getplatformproxy | ||
cf: request.cf, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I ran into this issue in my own codebase, I looked into it and wound up going with the non-null assertion operator (cf: request.cf!
) based on the Cloudflare docs, which explain that “the request.cf
object is not available in the Cloudflare Workers dashboard or Playground preview editor”: https://developers.cloudflare.com/workers/runtime-apis/request/#incomingrequestcfproperties
in every other case, the request.cf
object is defined and available, so I figured it seemed safe to assert that in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really keen for this to land! I do have a question, I haven't been able to find a good answer to this yet. How can we set |
Co-authored-by: Brooks Lybrand <[email protected]>
75740be
to
d9fcd55
Compare
They are not supported yet. But you can find some workarounds under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, thanks so much @edmundhung!
Is there already a port of this to RR7? @brookslybrand it seems like the type in |
This PR updates the cloudflare-workers template to use the Static Assets feature announced today.
You can learn more about Static Assets in the documentation.
Closes: #
Testing Strategy:
To test, please run
create-remix
with the template on my forkThen deploy it with wrangler by running:
Woohoo! Your Remix app is now running on Cloudflare Workers: https://template.remix-run.workers.dev