feat: use a single transport for fetchModule and HMR support

[sapphi-red]

Description

so that env providers not need to implement both fetchModule transport and HMR transport.

I think this PR still needs to polish the naming and probably better to introduce a interface that wraps HotChannel so that env providers does not need to care about internal conversion that is done for send and invoke (done).

The upper part is how it was before and the lower part is how it is after.

The diff of the environment implementation looks like sapphi-red/daienjo-architecture@966c60a...feat/try-single-transport (note that this diff includes two environments, service-worker and workerd)

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

I polished the interface so that it's more easier to implement for the env providers. I think we need a better name and check if this is not breaking a code that was working in 5.x.

[sheremet-va]

@bluwy pointed out the inconsistency in the naming of RunnerTransport in #16358 (comment)

Should we also standardise it here?

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 38b50bf: Open

suite	result	latest scheduled
astro	❌ failure	❌ failure
remix	❌ failure	❌ failure
sveltekit	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-environment-examples	❌ failure	✅ success
vitest	❌ failure	❌ failure

✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

[jamesopstad]

Would it be possible to add examples of what the API looks like before and after to the description? That would help with understanding the change.

[sapphi-red]

@jamesopstad Thanks for taking a look! The diff looks like this: sapphi-red/daienjo-architecture@966c60a...feat/try-single-transport (note that this diff includes two environments, service-worker and workerd)
Both of the env used in this example uses the newly exposed createWebSocketModuleRunnerTransport, but you can make a custom ModuleRunnerTransport as before.

[jamesopstad]

Thanks @sapphi-red. That helps a lot! I really like this proposal.

One thing that I'm wondering is if it might be possible to make it so that the transport can be added lazily? So, rather than providing { hot: true, transport: hot } you could provide { hot: true } and then add the transport by calling environment.setTransport(transport) or something similar.

We have this scenario where the WebSocket connection can't exist before the DevEnvironment constructor is called. We get around it by passing in an object and then mutating it later but it feels a bit hacky (https://github.com/flarelabs-net/vite-plugin-cloudflare/blob/7adf439fbd5e187c7419fe5686ed653191363d87/packages/vite-plugin-cloudflare/src/cloudflare-environment.ts#L82-L85). It seems you do something similar in your example (https://github.com/sapphi-red/daienjo-architecture/blob/577a508368380f728ac0941607c85ecf419c0f71/environment-pkgs/workerd/index.ts#L162). It would be nice if there was a cleaner solution for this.

The only other thing I'm wondering is if it might be possible to avoid the need for setInvokeHandler to make the API a bit cleaner. As far as I can tell setInvokeHandler is part of the public API but invokeModuleHandler is specific to your example. Does something equivalent to invokeModuleHandler need to be part of the public API or does that not make sense? Probably missing some context here so just trying to understand.

[sapphi-red]

One thing that I'm wondering is if it might be possible to make it so that the transport can be added lazily?

I guess that's possible. But I'd like to leave that for this PR as this PR is already quite large.

I'm wondering is if it might be possible to avoid the need for setInvokeHandler to make the API a bit cleaner.

HotChannel is an interface that is implemented by users and all methods in the interface are called by Vite. On the other hand, handleInvoke (a function that removes miniflare specific parts from invokeModuleHandler) is something that users will call. setInvokeHandler inverts that so that it can be in the HotChannel interface. This was a compromise for me so that transport related things are all inside the HotChannel. I would love to change it.

The other ways I can think of is to:

• expose handleInvoke from NormalizedHotChannel
  • users can use it like:

    const payload = (await request.json()) as HotPayload
    const result = await this.hot.handleInvoke(payload)
    return new MiniflareResponse(JSON.stringify(result))

  • the downside of doing this is that this requires users to know more about the difference of HotChannel and NormalizedHotChannel. Maybe that's fine?
• expose handleInvoke from DevEnvironment
  • users can use it like:

    const payload = (await request.json()) as HotPayload
    const result = await this.handleInvoke(payload)
    return new MiniflareResponse(JSON.stringify(result))

  • the downside of doing this is that the transport related things are leaked to DevEnvironment

[hi-ogawa]

• expose handleInvoke from NormalizedHotChannel

This sounds good to me 👍 It looks like we cannot hide the normalization entirely anyways, so adding this won't hurt hopefully.
(But again, I'm fine to iterate further in a separate PR.)

[sapphi-red]

I implemented NormalizedHotChannel::handleInvoke 👍

[sheremet-va]

Looks good to me 👍🏻

[pkg-pr-new]

Open in Stackblitz

pnpm add https://pkg.pr.new/vite@18362

commit: 65a23d2

[hi-ogawa]

Look good! I just double checked on my repo with pkg.pr.new