feat(templates/cloudflare-workers): update to ESM

[jacob-ebey]

Closes: #764

• [ ] Docs
• [ ] Tests

Testing Strategy:

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 6c0e0d30da694c6a6953a92d77c33918306d36a3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[huw]

This adds a lot of boilerplate. Has the Remix team considered updating createEventHandler or adding a new exported function to @remix-run/cloudflare-workers?

[MichaelDeBoey]

@huw Do you mean something like @ekosz is doing in #1207?

[huw]

Yeah, that seems closer to what I’d expect & it feels (very subjectively) more idiomatic.

The reason this PR removes the dependency on @remix-run/cloudflare-workers is because it completely supersedes it by inlining the functionality into the template. It would require a few changes to the docs to explain that @remix-run/cloudflare-workers is only appropriate for the old service-worker syntax, and that ESM users should copy from this template. My guess is that this isn’t what the Remix team want, unless you’re planning to move away from adapters in the long run?

I suspect it would be easier for end users to understand & upgrade if they only had to make a small change to their server.js file—@ekosz’s example is simply:

 import { createEventHandler } from "@remix-run/cloudflare-workers";

 import * as build from "../build";

 const handler = createEventHandler({ build })

 export default {
   fetch(request, env, context) {
     const event = { request, env, waitUntil: context.waitUntil.bind(waitUntil) };
     return handler(event);
   }
 }

[vstreame]

@MichaelDeBoey @jacob-ebey Any update on which direction we want to take here? We would love to be able to switch from Cloudflare pages to workers, but this is blocking us

[dario-piotrowicz]

I just wanted to mention that I just pulled down this branch and checked locally on a wrangler project and this PR does seem to work in its current state (compared to #1207 which has a few bits not fully working)

@MichaelDeBoey @jacob-ebey could we get this PR merged soon even if some small things can be improved and iterate over those details afterwards? (so that devs using Cloudflare workers can at least start using the ESM version sooner)

[huw]

@dario-piotrowicz FWIW this is just a change to the Workers template, not any of Remix's actual code. Remix is fully compatible with Workers today, users just need to copy the code in this template into their setups.

However, that requires a pretty specific setup. Users need to be wary of where their build is and update the reference in code if they reconfigure Remix, for one. And they have to use Wrangler's built-in bundler, or rebundle the Remix output themselves (since this code does not bundle dependencies into the final server code as-is). It's also using a deprecated config flag in remix.config.js to hack around the Remix compiler.

IMHO, as I said above, this is a very brittle approach to solving this problem & doesn't feel in line with the high standards both Remix & Cloudflare tend to aspire to for developer experience. And it would leave significant fragmentation in the Remix DX for users, because the instructions for integrating with ESM vs CJS would end up being very different--much more different than changing between Pages & Workers Sites, which is as simple as updating remix.config.js and a handful of lines in the server index file.

Again, the preferable solution would be to update the @remix-run/cloudflare-workers package. I gave this a crack today, but because the KV asset handler requires that __STATIC_CONTENT_MANIFEST import, getting interop with CJS is much trickier. I think the right approach might be to:

1. Create a new package (@remix-run/cloudflare-workers-esm or just cloudflare-esm) with ESM-specific code (i.e. tweaked #1207 code).
2. Update the Remix compiler to compile a new cloudflare-esm target as ESM with the neutral platform, while still bundling browser code.
3. Update the docs.

I really want to stress how unusable this PR would be in its current state & how much additional support load it will create for Cloudflare & Remix. I'm in a busy period rn but would gladly do this properly once I'm through that.

[dario-piotrowicz]

@huw thanks so very much for the nice clarification, yeah I understand that it is only template changes, but this is not at all straightforward to get done without checking out this PR, that's why I was asking to include it so to reduce the barrier to entry (maybe it could be added in a README, docs or something?)

Anyways I had no idea that this was brittle/using a deprecated flag, I thought that the only downside of this solution was the amount of boilerplate needed. Based on what you said I agree that adding something half-baked in at this stage would make things worse rather than better....

Thanks for taking the time to look at this, I'll patiently wait for you to have more time to look into this then 🙂

Also, I don't have much knowledge on how Remix works under the hook, but if you think I could be of any help here please let me know 🙏

[MichaelDeBoey]

@jacob-ebey @huw @dario-piotrowicz I'm wondering if you could obtain the same result with the new config properties that were exposed by #4841 🤔

[MichaelDeBoey]

@jacob-evey Closing this as @pcattori updated the cloudflare-workers template to ESM in #6650