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

Replace vite-node-miniflare with cloudflareDevProxy from @react-router/dev #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ariofrio
Copy link

@ariofrio ariofrio commented Dec 23, 2024

Issues with Cloudflare Templates

  1. Duplicate configuration between wrangler.toml and vite.config.ts for dev mode
  2. Complex vite.config.ts requiring manual CJS dependency configuration in ssr.optimizeDeps.include to work in dev mode
  3. Dependency on unmaintained @hiogawa/vite-node-miniflare for local dev mode
  4. Prerendering issues (#31) - to be fixed in PR fix: Cloudflare templates break when prerendering is enabled #45

Solution

Using cloudflareDevProxy from @react-router/dev:

  • Automatically pulls config from wrangler.toml for development
  • CJS dependencies in dev mode work without manual configuration
  • Automatically configures other ssr options or makes them unnecessary
  • Both cloudflare and cloudflare-d1 templates updated

@jacob-ebey: Any thoughts on why the templates weren't using cloudflareDevProxy initially?

Note: This supersedes #6 (does this for cloudflare template and also adds wrangler types support). That should be separate since we need to decide on npm scripts and .gitignore handling.

@itxch
Copy link

itxch commented Dec 27, 2024

@ariofrio with this change does the d1 example actually still work?
does it pick up "./workers/app.ts"?

@ariofrio
Copy link
Author

ariofrio commented Dec 27, 2024

@ariofrio with this change does the d1 example actually still work? does it pick up "./workers/app.ts"?

Ah, great catch. The cloudflare-d1 example doesn't currently work in this PR.

cloudflareDevProxy actually skips ./worker/app.ts altogether. By default, it passes bindings as loader and action context in the format {cloudflare: {env: {...}}} and allows users to provide a getLoadContext callback to customize that format. This works great if you're willing to use bindings by passing them explicitly as function arguments. This is what I do in my own project.

I'm unsure how to make this work with AsyncLocalStorage.run for implicit dependency injection, though, as the current cloudflare-d1 template does. Some options to consider:

  1. Add a callback option to cloudflareDevProxy to wrap execution of the request handler.
  2. Figure out if viteDevServer middleware can be used somehow instead of (1).
  3. Modify entry.server.tsx and wrap the call to renderToReadableStream.
  4. Stop using AsyncLocalStorage in the template in favor of explicit context.

@itxch
Copy link

itxch commented Dec 28, 2024

I'm unsure how to make this work with AsyncLocalStorage.run for implicit dependency injection, though, as the current cloudflare-d1 template does. Some options to consider:

Would it be worth in that case, having this PR just updates the cloudflare template, and open an issue with the above?
I'll be looking into ways to get it working with AsyncLocalStorage today too

@WonderPanda
Copy link

I'm super new to both Cloudflare and Remix run but am really interested in getting set up with best practices while I'm learning. Seeing all of the complex vite plugin config get removed in this PR definitely seems appealing but I'm not fully understanding how I would go about using D1 with these changes.

@diurivj
Copy link
Contributor

diurivj commented Jan 3, 2025

@WonderPanda I opened a PR on react-router-hono plugin that shows how simple is to use cloudflare and react router powered by a hono server, which claims to be the faster server for workers.

Here is the example project
https://github.com/rphlmr/react-router-hono-server/tree/main/examples/cloudflare/d1-drizzle

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.

4 participants