feat(remix-cloudflare-workers): support ES Module

[ekosz]

Hi there,

This is my first contribution to the Remix project so please let me know what additions you would like to see from this PR 😄 .

I've very interested in using features like Durable Objects in my Cloudflare workers, but to use them we need the ability to publish ESM versions of our workers. Remix has already(?) shipped the ability to generate native ESM, but we still need to update the @remix/cloudflare-workers package.

This change adds three commits to help us get there. Each commit explains what it does, but happy to further explain here as well.

Cheers!

P.S. This also fixes a bug where if the result of the handler is a Promise<Response> that rejects, it wouldn't have been caught by the try catch block due to the promise not being awaited.

Closes #764

[remix-cla-bot[bot]]

Hi @ekosz,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot[bot]]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[ekosz]

@ryanflorence Is there something I can do to improve this PR? Or is there a separate work stream happening to improve the Cloudflare story?

Just wondering if I should continue to open these types of PRs right now. Thanks!

[ryanflorence]

Is there something I can do to improve this PR?

Sorry, just been getting the company organized and the framework off the ground! We're finally able to spend some time in the PRs/Issues.

I'm going to assign this @jacob-ebey, I'm not as familiar w/ cloudflare workers.

[styxlab]

Any updates on this PR? Would like to see this progress further.

[MichaelDeBoey]

@ekosz Could you please rebase your branch onto latest dev & resolve conflicts?

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: c905446c087e571bc7c2ac169a3c1c3a2cd25cfa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/cloudflare-workers	Major
create-remix	Major
remix	Major
@remix-run/architect	Major
@remix-run/cloudflare	Major
@remix-run/cloudflare-pages	Major
@remix-run/deno	Major
@remix-run/dev	Major
@remix-run/eslint-config	Major
@remix-run/express	Major
@remix-run/netlify	Major
@remix-run/node	Major
@remix-run/react	Major
@remix-run/serve	Major
@remix-run/server-runtime	Major
@remix-run/vercel	Major

Not sure what this means? Click here to learn what changesets are.

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

[ekosz]

@MichaelDeBoey I've updated the PR (better 10 months later than never). This was my first time using changesets so I would love a review to make sure I did it correctly.

Thank you and excited to finally get this merged as this has been blocker for us for a long time now

[MichaelDeBoey]

@ekosz It seems like @jacob-ebey has a solution that only involves changes to the template in #4198.

Maybe it's a good idea to join forces on this & suggest changes to @remix-run/cloudflare-workers if still needed? 🤔

[ekosz]

@MichaelDeBoey That seems like a much better change than this one? I tried to keep this as contained as possible. Does it seem like that PR have a high likelihood of landing soon? If so it can supersede this one. Otherwise if it seems that PR may take a while to land may I suggest this be merged in the meantime? This is something people have wanted for a while.

[ekosz]

With the new comments on this PR, does that mean @MichaelDeBoey that the Remix team wants to go this direction instead? I just don't want to address these comments just to have the PR closed

[dario-piotrowicz]

With the new comments on this PR, does that mean @MichaelDeBoey that the Remix team wants to go this direction instead? I just don't want to address these comments just to have the PR closed

Hi @ekosz sorry about that, me and @petebacondarwin were just trying to get Remix to work with Cloudflare Worker ESM mode as that would be a very valuable thing to have, we've stumbled across this PR without noticing that there was already https://github.com/remix-run/remix/pull/4198 (and there seem to even be a third one https://github.com/remix-run/remix/pull/4401 😨).

I'll be checking out https://github.com/remix-run/remix/pull/4198 next to see if that solution work 🙂, as of this PR unfortunately it doesn't seem to work in its current state, because of the 2 comments we've added + another issue with the ASSET_NAMESPACE and ASSET_MANIFEST not being correctly passed to getAssetFromKV (but I totally understand you not fixing all the issue while being unsure of the state of this PR).

[dario-piotrowicz]

@MichaelDeBoey That seems like a much better change than this one? I tried to keep this as contained as possible. Does it seem like that PR have a high likelihood of landing soon? If so it can supersede this one. Otherwise if it seems that PR may take a while to land may I suggest this be merged in the meantime? This is something people have wanted for a while.

As you mentioned @ekosz this is something that people would have needed for a while and as time progresses more Cloudflare features assume and work better with the use of the (preferred) ESM mode, so it would be very good to have this issue fixed soon, @MichaelDeBoey please let me know if I can help in any way as I would be very happy to do so 🙏

[MichaelDeBoey]

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

If not, are you still interested in getting this one merged @ekosz?

If so, please rebase onto latest dev & resolve conflicts/remarks

[huw]

@MichaelDeBoey Nope, still need to add an additional handful of files (see solution here)—the new config options helped a tonne though and ultimately unblocked that work ❤️. The most idiomatic solution should be my one there, or the update this morning from @xanderberkein.

To make progress on this we just need agreement on who should own the adapter package and those discussions never happened (but you & I don’t work for CF or Remix so our options are limited here). IMHO this PR should be closed in favour of having that discussion, if we can ever get it to happen.

[MichaelDeBoey]

@huw We also still have @jacob-ebey's #4198 & #6229, which adds ESM-support to the CF Workers template without the need of changing the adapter

Could those be possible solutions as well you think?

[huw]

Yep, I weighed in on that debate here last time it came up (most of my points still apply, but the situation has improved a bit with the new config properties).

Given how much time has passed without much of a discussion between Remix & CF, it’s probably better to just merge #6229 when it’s done. I would prefer we create a new package for the reasons in that comment, but given that that seems politically impossible right now (not enough interest from either side), updating the template would at least provide user value sooner. I’ll leave some suggestions on that PR :)

[MichaelDeBoey]

@huw For what it's worth: I thought @jacob-ebey was also planning on only supporting ESM in v2 (or at least making it the default), so I guess the changes that are done in this PR could come in handy when implementing that 🤔

[xanderberkein]

I didn't realize the debate on who should own the ES Modules adapter was still ongoing. I just released my own adapter yesterday: https://github.com/xanderberkein/remix-cloudflare-module-workers.

From a DX perspective, I agree with @huw: I'd prefer a separate adapter (or even better - an update to the current cloudflare-workers adapter) that abstracts away the boilerplate otherwise needed in the template.

The service worker syntax does nothing you can't do with module workers, so there is no real need to keep the old service worker syntax around. As you can see from my adapter, the only change needed to convert the current cloudflare-workers adapter from a service worker to a module worker, is a few minor changes to the createEventHandler (https://github.com/xanderberkein/remix-cloudflare-module-workers/blob/main/src/worker.ts#L16), and some small adjustments to the template. The breaking change when switching to module workers would be that people would need to manually follow the three last steps described in the readme of my adapter, so we'd need to do a gradual switch with feature flags of course.

[brookslybrand]

I'm going to go ahead and close this PR since the original issue (#764) is closed by #6650

Thanks @ekosz for opening this up so long ago