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

docs(README) test: bindings and variables from wrangler.toml #83

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KaiSpencer
Copy link

@KaiSpencer KaiSpencer commented Feb 21, 2024

Adds some documentation to the readme on how pull from wrangler.toml when using vite dev server

Also adds an e2e test ensuring the functionality works

Related: #76 #39

@yusukebe
Copy link
Member

Hi @KaiSpencer !

Thanks!

First, you should separate the PR between fixing the README and adding tests.

For testing. It might be better to do the testing on @hono/vite-dev-server and not on this project:
https://github.com/honojs/vite-plugins/tree/main/packages/dev-server

Because @hono/vite-dev-server has the feature to handle Bindings. What do you think?

@KaiSpencer
Copy link
Author

Hi @KaiSpencer !

Thanks!

First, you should separate the PR between fixing the README and adding tests.

For testing. It might be better to do the testing on @hono/vite-dev-server and not on this project: https://github.com/honojs/vite-plugins/tree/main/packages/dev-server

Because @hono/vite-dev-server has the feature to handle Bindings. What do you think?

Hey @yusukebe,

Happy to separate the PR!

Yes I can understand how the tests would be better in the vite plugin, given honox devServer configuration option is exactly passed through and it is not manipulated/changed at all.

I will reduce this PR to be just docs changes, and make a separate PR over in that repo for a e2e test that covers this functionality

… from wrangler.toml"

This partially reverts commit 66fb0d7.
Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

Maybe this should be in "Deployment", but you might note that Cloudflare Pages does not read wrangler.toml in production, Remix does.

https://github.com/remix-run/remix/tree/main/templates/vite-cloudflare#deployment

return {
plugins: [
honox({
devServer: {
Copy link
Member

Choose a reason for hiding this comment

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

Since this fix made the type correct, it would be easier to specify "env and onServerClose" as one plugin:

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

Copy link
Author

Choose a reason for hiding this comment

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

oo, nice. Will update to match :D

@yusukebe
Copy link
Member

Hi @KaiSpencer

Thanks! I've commented.

@KaiSpencer
Copy link
Author

Maybe this should be in "Deployment", but you might note that Cloudflare Pages does not read wrangler.toml in production, Remix does.

https://github.com/remix-run/remix/tree/main/templates/vite-cloudflare#deployment

Yeah, I considered if to put this into Deployment or to make Development, and changed my mind a few times along the way! I didnt notice that with remix, I gues it fits to both categories. Happy with with your direction on where you would like this

@KaiSpencer
Copy link
Author

Whilst writing this, I have come across this issue in the dev-server vite plugin.
honojs/vite-plugins#92

It does not change how this should be documented, we can carry on the testing discussion over there and just keep this to the docs change

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