bug(dev-server): with multiple configuration methods, env is overwritten

[KaiSpencer]

Whilst looking at making a PR from honojs/honox#83, I have noticed that the value for env used by the dev-server plugin is overwritten depending on the configuration used.

See:

              let env: Env = {}
              console.log('OPTIONS', options)

              if (options?.env) {
                if (typeof options.env === 'function') {
                  env = await options.env()
                } else {
                  env = options.env
                }
              } else if (options?.cf) {
                env = await cloudflarePagesGetEnv(options.cf)()
              }

              if (options?.plugins) {
                for (const plugin of options.plugins) {
                  if (plugin.env) {
                    env = typeof plugin.env === 'function' ? await plugin.env() : plugin.env
                  }
                }
              }

if you supply via root env, and then also cf (albeit deprecated), and then multiple plugins, only the values in the last plugin will be passed through.

Is this intended behaviour? Or should each configuration option extend env

I found this by adding a second plugin in the e2e tests, see below.

export default defineConfig(async () => {
  const { env, dispose } = await getPlatformProxy({ configPath: './wrangler.toml' })

  return {
    plugins: [
      devServer({
        entry: './mock/worker.ts',
        exclude: [...defaultOptions.exclude, '/app/**'],
        plugins: [
          { onServerClose: dispose, env },
          pages({
            bindings: {
              NAME: 'Hono',
            },
          }),
        ],
      }),
    ],
  }
})

This results in only the NAME binding passed through, and those set in wrangler.toml picked up by getPlatformProxy being ignored.

This can be worked around by extending the bindings property of the cloudflare-pages package like so

pages({
            bindings: {
              NAME: 'Hono',
              ...env,
            },
          }),

Is it intended to be able to supply multiple methods of configuration? If so, this code can be updated to extend rather than assign.

[yusukebe]

@KaiSpencer

Thanks for pointing this out!

Honestly, I don't think it's a good API to have env and onServerClose for each of multiple plugins. I have an idea, please wait a bit!

[KaiSpencer]

@KaiSpencer

Thanks for pointing this out!

Honestly, I don't think it's a good API to have env and onServerClose for each of multiple plugins. I have an idea, please wait a bit!

Of course! If there is anything I can help with let me know, but for now I will leave it with you

[yusukebe]

Hi @KaiSpencer

I've thought about it overnight and we don't need to change the API right now.

Is it intended to be able to supply multiple methods of configuration? If so, this code can be updated to extend rather than assign.

Yes. If two envs are specified, the values of the two envs should be merged, not assigned.

If you have the time I would be glad if you could make a PR for this!

[yusukebe]

@KaiSpencer

Thanks for making the PR, but I think we should make an adapter option and recommend using it instead of plugins[]; what do you think?

import honox from 'honox/vite'
import { defineConfig } from 'vite'
import { getPlatformProxy } from 'wrangler'

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

This means that for a development server, it is better to have only one env then I thought adapter was better than the concept of plugins. I thought adapter would be better.

Of course, we must leave the plugins[] option to the compatibility.

Sorry for changing my opinion and the time taken.

[KaiSpencer]

@KaiSpencer

Thanks for making the PR, but I think we should make an adapter option and recommend using it instead of plugins[]; what do you think?

import honox from 'honox/vite'
import { defineConfig } from 'vite'
import { getPlatformProxy } from 'wrangler'

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

This means that for a development server, it is better to have only one env then I thought adapter was better than the concept of plugins. I thought adapter would be better.

Of course, we must leave the plugins[] option to the compatibility.

Sorry for changing my opinion and the time taken.

No problem @yusukebe,

I'll take a look and make the changes

[KaiSpencer]

@yusukebe Do you want any logic changed with the current bug of env overwriting? or just leave that as is