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

Cloudflare support for Vite #8531

Merged
merged 28 commits into from
Jan 25, 2024
Merged

Cloudflare support for Vite #8531

merged 28 commits into from
Jan 25, 2024

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Jan 16, 2024

This currently relies on wrangler@beta, so let's wait to merge this until the next stable release of wrangler that includes getBindingsProxy. CF core team says that should happen next week or so.

Usage

// vite.config.ts
import {
  unstable_vitePlugin as remix,
  unstable_vitePluginAdapterCloudflare as cloudflare,
} from "@remix-run/dev";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [
    remix({
      adapter: await cloudflare(),
    }),
  ],
});

The cloudflare adapter reads bindings from wrangler.toml and sets them on the env field of load context:

// app/routes/_index.tsx

// ... some code ...

const key = "__my-key__";

export async function loader({ context }: LoaderFunctionArgs) {
  const { MY_KV } = context.env;
  const value = await MY_KV.get(key);
  return json({ value });
}

// ... some more code ...

TODO

  • build script: ssr conditions as part of adapter API
  • docs
  • tests
  • changeset

Copy link

changeset-bot bot commented Jan 16, 2024

🦋 Changeset detected

Latest commit: 132fc21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/cloudflare-pages Patch
@remix-run/dev Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
@remix-run/testing Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pcattori pcattori marked this pull request as draft January 16, 2024 19:24
@cloudkite
Copy link

Exciting 🙌 does the build target workers or pages? Or can we choose the build target? Thanks!

@pcattori
Copy link
Contributor Author

Exciting 🙌 does the build target workers or pages? Or can we choose the build target? Thanks!

Pages. CF core team flagged that Workers is now deprecated since Pages now is able to do everything Workers could do

@sergiodxa
Copy link
Member

@pcattori I would like to try this on my own website since I deploy to CF Pages, how could I do it? Would it be enough if I use patch-package to patch the Vite plugin to include the changes here?

@jfsiii
Copy link

jfsiii commented Jan 17, 2024

@pcattori thanks for highlighting "do not use Worker Sites for new projects".

We have a production app using Sites and I'd love to adopt Vite without having to also switch to Pages. It's clear we'll need to eventually but that prerequisite pushes Vite so much further away.

Would supporting Workers and Pages be a lot of extra work/complexity/etc? Perhaps Workers could be supported for some bridge period then removed later?

@pcattori
Copy link
Contributor Author

@sergiodxa I'll include instructions for trying things out once its in a good state for that. Right now there are a couple pieces missing.

@jfsiii I haven't looked into how much work supporting CF Workers would be. There's a decent chance that this work will already support CF Workers but not something we test for right now, so would love feedback from y'all if you try it out.

@pcattori pcattori force-pushed the pedro/vite-cloudflare branch 2 times, most recently from 6da77a9 to 3e41c29 Compare January 18, 2024 22:26
@pcattori
Copy link
Contributor Author

CF merged the getBindingsProxy PR and that is now available in latest beta release of wrangler. They said stable release should be sometime next week, so let's wait for that and then we should be able to nuke the entire .cloudflare/ dir in the template. It also means we can read CF bindings from wrangler.toml instead of requiring duplication of those bindings in vite.config.ts!

Comment on lines 4 to 9
const build = async (): Promise<ServerBuild> => {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
// eslint-disable-next-line import/no-unresolved
return import("../build/server");
};
Copy link
Contributor Author

@pcattori pcattori Jan 20, 2024

Choose a reason for hiding this comment

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

Getting type-safe access to the build has come up a couple times.

@markdalgleish I'm tempted to design a getBuild API that returns Promise<ServerBuild | undefined> to avoid this ugly boilerplate. Server bundles complicate things slightly, though we could just return undefined when server bundles are present.

const build = getServerBuild()
if (!build) throw Error("gotta run `remix vite:build` first!")

export onRequest = createRequestHandler({ build })

Plus this means you can change your output dir in vite.config.ts without needing to update code in server.ts or other scripts that rely on its location.

Copy link
Contributor Author

@pcattori pcattori Jan 20, 2024

Choose a reason for hiding this comment

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

Writing the above snippet made me think that we should just include the throw Error part within getServerBuild and return Promise<ServerBuild> instead.

Copy link
Contributor Author

@pcattori pcattori Jan 22, 2024

Choose a reason for hiding this comment

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

That said, this is something we can consider after Vite is stabilized.

@sergiodxa
Copy link
Member

Since the bindings are set automatically, will context.env get correct types for the bindings? Like context.env.MY_KV bring a KVNamespace? Or we will need it still override AppLoadContext?

Comment on lines +14 to +16
"@remix-run/cloudflare": "*",
"@remix-run/cloudflare-pages": "*",
"@remix-run/react": "*",
Copy link
Contributor

@hi-ogawa hi-ogawa Jan 22, 2024

Choose a reason for hiding this comment

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

Currently Remix includes @remix-run/node in optimizeDeps but this template doesn't have direct dependency on it. This might cause Vite's warning for non-flat node_modules like pnpm where @remix-run/node is not visible from root project.

I have a similar case where I use @remix-run/server-runtime directly without @remix-run/node and I've seen this warning on startup:

Failed to resolve dependency: @remix-run/node, present in 'optimizeDeps.include'

I'm not sure what's the best way to avoid this within Remix (also knowing that the necessity of this optimizeDeps might be only for non-standard setup during tests), but for now, what I did was to exclude manually by custom plugin config hook https://github.com/hi-ogawa/demo-remix-unstable-vite-cloudflare-workers/blob/2bdd6a4e15dc99584aa23897a5c1a40d7b185f6e/vite.config.ts#L15-L17

@pcattori
Copy link
Contributor Author

Since the bindings are set automatically, will context.env get correct types for the bindings? Like context.env.MY_KV bring a KVNamespace? Or we will need it still override AppLoadContext?

The bindings are defined in a TOML file (wrangler.toml) so there's no way to get automatic types for it without type gen. Not something I'm inherently opposed to, but we don't have type gen as part of the scope for the "stabilizing Vite" work.

@cloudcreatr
Copy link

How do I access the D1 database? Am i supposed to make wrangler file or pass it through wrangler dev --d1 flag....

@pcattori
Copy link
Contributor Author

pcattori commented Jan 22, 2024

How do I access the D1 database? Am i supposed to make wrangler file or pass it through wrangler dev --d1 flag....

The wrangler dev --d1 flag only sets D1 when running wrangler. But with remix vite:dev, wrangler is never run. So if you want a consistent experience across Vite Dev Server, wrangler dev, and your deployment, its best to define it within wrangler.toml.

🚨 As of today, the latest stable version of wrangler (3.23.0) does not support wrangler.toml for CF pages. This feature is available on wrangler@beta or the next release of wrangler (presumably 3.24)

@cloudcreatr
Copy link

@pcattori cf pages doesn't support toml file you said.. but the movie demo which Ryan launched uses toml file and it works fine and he applied migration that are visible in local db also.. but i think as you said.. toml doesn't work and the latest remix and cloudflare is kind off broken.. when i apply migration. The changes is not reflected in local db even after passing flags and creating a toml file as recommended by cf docs.. i wanted to know.. there's no server file in app dir.. how is streaming handled.. also.. i wanted to setup this locally with binding for my project just to see and work and play around.. even if it's beta.. how can I set up like . Thanks for your work

@pcattori
Copy link
Contributor Author

@pcattori cf pages doesn't support toml file you said.. but the movie demo which Ryan launched uses toml file and it works fine and he applied migration that are visible in local db also.. but i think as you said.. toml doesn't work and the latest remix and cloudflare is kind off broken.. when i apply migration. The changes is not reflected in local db even after passing flags and creating a toml file as recommended by cf docs.. i wanted to know.. there's no server file in app dir.. how is streaming handled.. also.. i wanted to setup this locally with binding for my project just to see and work and play around.. even if it's beta.. how can I set up like . Thanks for your work

CF core team mentioned that CF pages reading from wrangler.toml before v3.24 was an accidental implementation detail and may not read all values from wrangler.toml so something may have just worked, but not guaranteed. For how streaming is setup, checkout the default entry.server.tsx for CF.

@cloudcreatr good questions, but to keep this conversation focused on the changes introduced in this PR, please ask any follow-up questions about how CF + Remix generally works in our Discord. Thanks!

Comment on lines -9 to -10
<docs-warning>Note that Cloudflare is not yet supported when using Vite.</docs-warning>

Copy link
Member

Choose a reason for hiding this comment

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

😍

@pcattori pcattori changed the title WIP: CloudFlare support for Vite Cloudflare support for Vite Jan 25, 2024
@pcattori pcattori marked this pull request as ready for review January 25, 2024 19:59
@pcattori pcattori merged commit d74514a into dev Jan 25, 2024
9 checks passed
@pcattori pcattori deleted the pedro/vite-cloudflare branch January 25, 2024 20:18
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jan 25, 2024

# Wrangler
remix vite:build # build app before running wrangler
wranger pages dev ./build/client
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be 'wrangler'?

Copy link
Contributor Author

@pcattori pcattori Jan 26, 2024

Choose a reason for hiding this comment

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

It is using wrangler via wrangler pages dev, not sure I understand your question.

Copy link
Contributor

@reichhartd reichhartd Jan 26, 2024

Choose a reason for hiding this comment

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

Typo wranger instead of wrangler 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh now I see it 😅 yep you're right

@tsteckenborn
Copy link

tsteckenborn commented Jan 27, 2024

Am I doing something stupid, or is running it in dev via "remix vite:dev", currently always crashing / exiting completely if a route is not matching anything?

No routes matched location "..." 
/Users/.../Documents/div/.../node_modules/.pnpm/[email protected]/node_modules/wrangler/wrangler-dist/cli.js:29572
throw a;
        ^
TypeError: Cannot read properties of undefined (reading 'get')

@pcattori
Copy link
Contributor Author

Am I doing something stupid, or is running it in dev via "remix vite:dev", currently always crashing / exiting completely if a route is not matching anything?

No routes matched location "..." 
/Users/.../Documents/div/.../node_modules/.pnpm/[email protected]/node_modules/wrangler/wrangler-dist/cli.js:29572
throw a;
        ^
TypeError: Cannot read properties of undefined (reading 'get')

@tsteckenborn Can you provide a repro in stackblitz? My guess is that something is not playing nice with pnpm

@patdx
Copy link

patdx commented Jan 30, 2024

What's the recommended way to customize the getLoadContext in the vite Cloudflare mode?

For example I was attaching this cache adapter on each request: https://github.com/AdiRishi/cachified-adapter-cloudflare-kv

Should I create a custom server for dev mode and edit the getLoadContext in the catch-all route for prod mode? Or create an entry.server.ts file?


Update: For prod mode I solved as above. For dev mode, I ended up modifying the adapter in vite.config, but it feels like like I'm not supposed to be able to do this. Also I can't add any per-request properties to the context this way:

export default defineConfig({
  plugins: [
    remix({
      adapter: async () => {
        const { viteConfig, loadContext } = await cloudflare()();

        const env = loadContext.env; // KVDATA, D1DATA

        const ctx = {
          ...env,
          CACHIFIED_KV_CACHE: cloudflareKvCacheAdapter({
            kv: env.KVDATA as any,
          }),
          drizzle: createDrizzle(env.D1DATA),
          executionCtx: {
            waitUntil() {
              // no-op
            },
            passThroughOnException() {
              // no-op
            },
          },
        };

        return {
          viteConfig,
          loadContext: ctx,
        };
      },
    }),
  ],
});

@pcattori
Copy link
Contributor Author

@patdx there's still a couple things to figure out for a general way loadContext since it affects all possible servers, not just CF. What you're doing seems like an excellent workaround in the meantime.

Copy link
Contributor

🤖 Hello there,

We just published version 2.6.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@cloudkite
Copy link

@pcattori would be nice if this exposed both configPath and persist from: https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/api/integrations/bindings/index.ts#L19

This would be great for monorepos where the wrangler.toml and persisted data might live in the parent/root dir, which is not necessarily the same dir as vite.config.ts

Copy link
Contributor

github-actions bot commented Feb 1, 2024

🤖 Hello there,

We just published version 2.6.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Feb 1, 2024
@tsteckenborn
Copy link

tsteckenborn commented Feb 2, 2024

... solved. Issue was in front of the keyboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.