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

update cloudflare-d1 template to not use "@hiogawa/vite-node-miniflare" #63

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

brookslybrand
Copy link
Contributor

@brookslybrand brookslybrand commented Jan 17, 2025

This is an attempt to address #52, followup on #6, and fill in the gaps in #44

Jacob Paris also has a good article on using drizzle with Remix that does not use the loadContext.

Long term the React Router dev poxy should be able to be replaced by the cloudflare plugin/Vite Environment API. Contributions are appreciated when there is a better solution available.

Additionally, this PR improves the deployment instructions (see #58 for original implementation)

Comment on lines 40 to 45
cloudflareDevProxy({
getLoadContext({ context }) {
return getLoadContext(
// @ts-expect-error - context types are unaware of the proxied values
context.cloudflare
);
Copy link

@caprolactam caprolactam Jan 19, 2025

Choose a reason for hiding this comment

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

Suggested change
cloudflareDevProxy({
getLoadContext({ context }) {
return getLoadContext(
// @ts-expect-error - context types are unaware of the proxied values
context.cloudflare
);
cloudflareDevProxy<CloudflareEnvironment, Record<string, unknown>>({
getLoadContext({ context }) {
return getLoadContext(context.cloudflare);

and change load-context.ts to( to avoid the ctx type mismatch between wrangler and production):

  export interface AppLoadContext {
    cloudflare: {
      env: CloudflareEnvironment;
-      ctx: ExecutionContext,
+      ctx: Omit<ExecutionContext, "props">;
    };
    db: DrizzleD1Database<typeof schema>;
  }

I think ctx.props is not important because it is not mentioned in the source and its type is any in @cloudflare/worker-types.

Choose a reason for hiding this comment

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

I think the ideal code would look like this:

// Env is generated by running `wrangler types`
cloudflareDevProxy<Env>({
  getLoadContext({ context, request }) {
    return getLoadContext({ context, request }),
  }
})

However, ts expects cloudflareDevProxy to take two type arguments, so throwing an error.

The second argument specifies details for mocking of Request's cf property, so it is generally managed by the remix-run team or wrangler. Therefore, I think cloudflareDevProxy types could be modified as follows:

https://github.com/remix-run/react-router/blob/d47f65d0abdf2ccfffb0d2dfd44a44da0c6a4659/packages/react-router-dev/vite/cloudflare-dev-proxy.ts#L38

- export const cloudflareDevProxyVitePlugin = <Env, Cf extends CfProperties>(
+ export const cloudflareDevProxyVitePlugin = <Env, Cf extends CfProperties = CfPropeties>(

Tbh, it might be simple and easy to follow the code in the Remix doc to avoid ts errors.
(like: cloudflareDevProxy({ getLoadContext }))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caprolactam, thanks for all the thoughts! I think omitting the props is probably fine as well

Would love your thoughts on my changes. If you think it's good that's good enough for me to go ahead and merge

@brookslybrand brookslybrand merged commit 58177f3 into main Jan 22, 2025
4 checks passed
@brookslybrand brookslybrand deleted the brooks/cf-d1-update branch January 22, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants