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

[@hono/vite-dev-server] Proposal for Specialized Plugin: Cloudflare Workers/Wrangler Integration #25

Closed
Code-Hex opened this issue Nov 11, 2023 · 21 comments

Comments

@Code-Hex
Copy link

Related issue: #24

@hono/vite-dev-server currently supports development servers for most edge environments. However, as we attempt to address issues specific to platforms like Cloudflare Workers/Wrangler, such concerns become more prominent. Similarly, when it comes to features tailored for other edge environments, this can lead to a situation where, for those who want to utilize the Wrangler environment, this plugin becomes overly complex.

Therefore, I have created this issue with the idea that it would be beneficial to first extract specific functionality for Cloudflare Workers/Wrangler as a dedicated plugin. I would appreciate your input on this matter.

@Code-Hex Code-Hex changed the title Proposal for Specialized Plugin: Cloudflare Workers/Wrangler Integration [@hono/vite-dev-server] Proposal for Specialized Plugin: Cloudflare Workers/Wrangler Integration Nov 11, 2023
@navaru
Copy link

navaru commented Nov 11, 2023

It would be helpful if you could describe your desired workflow. What should be handled by Vite and what should be handled by Wrangler?

For example, for a client, I've implemented a Vite plugin that runs a dev server for their specific needs with the following config:

import { defineConfig } from "vite"
import tsConfigPaths from "vite-tsconfig-paths"
import cloudflare from "@acme/vite-plugin-cloudflare"

export default defineConfig(({ command, mode, ssrBuild }) => ({
  plugins: [
    tsConfigPaths(),

    cloudflare({
      pages: {
        name: "service/web",
        d1: [{ handle: "db", id: "..." }],
        kv: [
          { handle: "settings", id: "..." },
          { handle: "agents", id: "..." },
          { handle: "jobs", id: "..." },
        ],
      },
      workers: [
        {
          name: "service/identity",
          file: "./workers/identity.ts",
          d1: [{ handle: "db", id: "..." }],
          kv: [{ handle: "settings", id: "..." }],
        },
        {
          name: "service/payments",
          file: "./workers/payments.ts",
          d1: [{ handle: "db", id: "..." }],
          kv: [
            { handle: "transactions", id: "..." },
            { handle: "settings", id: "..." },
          ],
        },
      ],
    }),
  ],
}))

During Vite serve (vite dev) all workers run in Miniflare, and at build time the wrangler.toml is generated for the built worker. They use the wrangler CLI only for deployments and general settings.

@Code-Hex
Copy link
Author

Code-Hex commented Nov 11, 2023

@navaru As linked in the related issue, I hope that the vite plugin will recognize environment variables and various infrastructure bindings specified in TOML.

I plan to use --no-bundle with wrangler and intend to bundle both the server and client code using vite.

In other words, the suggestions you have provided don't meet the requirements I am looking for 😢

@navaru
Copy link

navaru commented Nov 11, 2023

I am a bit confused about what you're trying to achieve, as Cloudflare Pages don't use a wrangler.toml file.

In the title of this issue, you mention the plugin @hono/vite-dev-server, which is designed to be used by vite dev.

If you want to build for Cloudflare Pages you need to use @hono/vite-cloudflare-pages, which is a two-step process, as mentioned here: build a client, and does not require a wrangler.toml file.

If you plan on using Cloudflare Workers for Worker Sites, this is discouraged by Cloudflare:

Use Cloudflare Pages for hosting fullstack applications instead of Workers Sites. Do not use Workers Sites for new projects.

@Code-Hex
Copy link
Author

Code-Hex commented Nov 12, 2023

@navaru Yes, I'm planning to use for Workers Site. So I didn't mention about it.

I know your opinion but This is the best solution for us. I explain the reason below:

  1. Our application is running on Cloudflare Workers.
  2. Our application is originally only a backend, so we think it will be difficult to switch to Pages.
  3. There are three workers. They are prepared as dev, staging, and production environments respectively.
  4. Pages currently does not have a way to split the backend like this.
  5. If you use Workers, static site uploads will be R2 or Workers Site.
  6. R2 cannot upload directories using Wrangler, but Workers Site can.

If you plan on using Cloudflare Workers for Worker Sites, this is discouraged by Cloudflare

Our Cloudflare Workers are built following this instruction:

https://developers.cloudflare.com/workers/wrangler/environments/

@yusukebe
Copy link
Member

Hi @Code-Hex!

Like @navaru, I believe you're referring to Cloudflare Pages, but it seems you're actually thinking about Cloudflare Workers.

There are several approaches to better support Cloudflare Workers:

  1. Create a build plugin for Workers, similar to this one.
  2. Add a feature to read wrangler.toml options in the Vite dev-server. I'm not sure about the implementation; it might require a plugin structure?

This issue affects all platforms, including Workers and Pages:

  • The runtime environment is not always ideal. For example, when creating a Pages application and using Vite in development, the environment is actually "Node.js". If you use getRuntimeKey() in hono/adapter, it returns node. There's also the issue of missing APIs, like Cache API and URLPattern, which aren't implemented in Node.js but are available in workerd.

Regarding this, it's worth discussing. However, I think the current pattern is fine, as Hono apps tend to be isomorphic. If we can use Bindings as they are, it should be sufficient.

@yusukebe
Copy link
Member

I believe it would be better to implement the platform-dependent issues as a plugin structure for the dev server. If feasible, cf bindings by Miniflare could be one of the plugins. However, as mentioned above, I currently don't have a clear idea on how to implement this.

@naporin0624
Copy link
Contributor

@Code-Hex @yusukebe

I am wondering if this implementation can be achieved with reference to the vitest-miniflare-environment implementation.

The vitest-miniflare-environment reads the contents of wrangler.toml and automatically sets bindings as the output of getMiniflareBindings.

I was wondering if this part could be applied to a vite plugin.

I looked at how vitest-miniflare-environment gets bindings.

The vitest-miniflare-environment internally executes createMiniflareEnvironment of shared-test-environment, which passes the BindingPlugin to MiniflareCore. BindingPlugin is passed to MiniflareCore.
https://github.com/cloudflare/miniflare/blob/master/packages/shared-test-environment/src/index.ts#L27

The important part is the getBindings method of MiniflareCore. This method converts each of the bindings in wrangler.toml into a module that can be executed by cloudflare workers.
https://github.com/cloudflare/miniflare/blob/master/packages/core/src/index.ts#L365
https://github.com/cloudflare/miniflare/blob/master/packages/core/src/index.ts#L1045

I ported this getBindigns method to @hono/vite-dev-server and was able to get bindings in the fetch function.

MiniflareCore was initialized using the implementation of vitest-environment-miniflare and shared-test-environment as reference, so the code used for test is included and needs to be modified.

https://github.com/cloudflare/miniflare/blob/master/packages/vitest-environment-miniflare/src/index.ts#L86

Code for a plugin that runs getBindings via MiniflareCore and passes bindings to a fetch function via vite
https://github.com/naporin0624/cloudflare-workers-vite-plugin-test

@Code-Hex
Copy link
Author

Code-Hex commented Nov 13, 2023

@naporin0624 Thank you for providing your idea!

@yusukebe I've tried to implement it easily in the following PR. I have confirmed that d1 and kv are working correctly.

#26

I am copying a lot of code from workers-sdk. This is because some of the code defined in workers-sdk is not provided as a package. I thought it would be necessary to follow along in this regard.

@yusukebe
Copy link
Member

@naporin0624

Thanks!

I'm not sure if I fully understand, but here are my thoughts. Your approach referring to vitest-miniflare-environment is very interesting. However, vitest-miniflare-environment is only for Miniflare v2. The latest version is v3, and its repository has been moved to the workers-sdk:

https://github.com/cloudflare/workers-sdk/tree/main/packages/miniflare

With v3, we can use Miniflare#get*() like the getBindings method of MiniflareCore which you mentioned.

https://github.com/cloudflare/workers-sdk/tree/main/packages/miniflare#class-miniflare

So, our challenge is how to handle wrangler.toml.

@Code-Hex is working on this, so let's follow their lead.

@naporin0624
Copy link
Contributor

@yusukebe

OK!
I had forgotten that it was wrangler v2. Thank you very much.

@Code-Hex
Copy link
Author

MEMO: I just noticed that we don't need to use workers-sdk code. unstable_dev has the option to read wrangler.toml.

@Code-Hex
Copy link
Author

Code-Hex commented Dec 9, 2023

I was able to allocate some time, so I attempted to implement it using 'wrangler unstable_dev.'

The potential benefit of this approach was that I wouldn't have to keep track of updates to 'wrangler' myself. However, the fetch returned by the unstable_dev function is not part of the Worker's signature; it remains a client-side fetch, so it couldn't be used here. There might have been a possibility if Cloudflare Pages' plugins were handling client-side signatures.

I will proceed with the approach outlined in this PR.

@cloudkite
Copy link

BTW not sure if you are already aware but this should now be much easier to implement as there’s a new getBindingsProxy function in Miniflare cloudflare/workers-sdk#4523 so could be a great opportunity to simplify and remove a lot of code

@izznatsir
Copy link

izznatsir commented Jan 23, 2024

getBindingsProxy has been released to stable version of wrangler.

@Code-Hex
Copy link
Author

I will try to fix this using the API!

@yusukebe
Copy link
Member

yusukebe commented Jan 24, 2024

Hey @Code-Hex

Please wait a minute!

Now I am thinking of creating a "cloudflare plugin" with getBindingsProxy for the dev server. It would cover not only Workers but also Pages. With it I can deprecate the current "cloudflare-pages" plugin.

I am going to create that draft, and I would like @Code-Hex to review it.

@Code-Hex
Copy link
Author

@yusukebe sounds good!
Thank you so much!

@yusukebe
Copy link
Member

I need your help, but since this is a Cloudflare feature, let me, a Cloudflare employee, do it:)

@yusukebe
Copy link
Member

yusukebe commented Feb 8, 2024

I think we should wait a little longer on this issue.

The new version, 5.1 of Vite has been released.

https://vitejs.dev/blog/announcing-vite5-1.html

With this version, Vite is now experimentally supporting Runtime other than Node.js. This means that Miniflare/workerd environments can be integrated into Vite using Vite's API. We Cloudflare may make Miniflare/wokred runtime official. It is more efficient and preferred than having each framework support Cloudflare individually.

So, let's wait and see.

@yusukebe
Copy link
Member

Hey @Code-Hex and others!

Now, we can use the new API getPlatformProxy() in Wrangler. This will automatically read variables from wrangler.toml. You can integrate it into @hono/vite-dev-server without creating a plugin. Try it!

import devServer from '@hono/vite-dev-server'
import { defineConfig } from 'vite'
import { getPlatformProxy } from 'wrangler'

export default defineConfig(async () => {
  const { env, dispose } = await getPlatformProxy()
  return {
    plugins: [
      devServer({
        env,
        plugins: [
          {
            onServerClose: dispose
          }
        ]
      })
    ]
  }
})

@Code-Hex
Copy link
Author

I close this PR once!
continue discussion on the issue #24

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

No branches or pull requests

7 participants
@yusukebe @navaru @cloudkite @Code-Hex @izznatsir @naporin0624 and others