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

ekosz · 2021-12-23T00:00:44Z

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 · 2021-12-23T00:00:46Z

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 hello@remix.run.

Thanks!

- The Remix team

remix-cla-bot · 2021-12-23T00:08:49Z

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

ekosz · 2022-01-13T02:07:59Z

@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 · 2022-02-11T05:42:36Z

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 · 2022-04-25T15:50:15Z

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

MichaelDeBoey · 2022-06-03T00:02:53Z

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

changeset-bot · 2022-09-26T04:35:52Z

🦋 Changeset detected

Latest commit: c905446

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

The newer version of the library now supports ES Modules. See: https://github.com/cloudflare/kv-asset-handler/releases/tag/v0.2.0

When working with Cloudflare ES Module workers[0] we no longer have access to a `FetchEvent` object. Instead we're given three parameters, `request`, `environment`, and `context`. This is at odds with the "Service Worker" API where the environment vars are global and the given event has a request and helper properties. To bridge the gap between the two, this change adds a new type `Evt` and allows that to be passed in lieu of a `FetchEvent`. When this is the case the user is expected to create the `Evt` object themselves. Here is an example: ```ts import { createEventHandler } from "@remix-run/cloudflare-workers"; import type { Evt } from "@remix-run/cloudflare-workers"; import * as build from "../build"; type Environment = Record<string, unknown>; const handler = createEventHandler({ build }); export default { fetch(request: Request, env: Environment, context: ExecutionContext) { const event: Evt<Environment> = { request, env, waitUntil: context.waitUntil.bind(context) }; return handler(event); } } ``` This is very similar what the `@cloudflare/kv-asset-handler` did in their 0.2.0 release[1]; [0] - https://blog.cloudflare.com/workers-javascript-modules/ [1] - cloudflare/kv-asset-handler@79ebac4

This adds a section explaining how to make the worker work in ESM mode.

ekosz · 2022-09-26T04:45:44Z

@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 · 2022-09-26T13:24:36Z

@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 · 2022-09-26T13:36:41Z

@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.

huw

I gave this a crack on my machine—it’s still missing a few things before it will work. I think this is a preferable solution to the template change with a few tweaks. (I am not a Remix contributor, just a dev that also wants this 😌)

huw · 2022-10-13T13:24:59Z

packages/remix-cloudflare-workers/worker.ts

 /**
 * A function that returns the value to use as `context` in route `loader` and
 * `action` functions.
 *
 * You can think of this as an escape hatch that allows you to pass
 * environment/platform-specific values through to your loader/action.
 */
-export type GetLoadContextFunction = (event: FetchEvent) => AppLoadContext;
+export type GetLoadContextFunction = (event: FetchEvent | Evt) => AppLoadContext;


I think it would make sense to parameterise <Environment> for this & dependent functions, to allow the user to easily type it. So:

export type GetLoadContextFunction<Environment = unknown> = (event: FetchEvent | Evt<Environment>) => AppLoadContext;

huw · 2022-10-13T13:30:40Z

templates/cloudflare-workers/README.md

+```
+
+**Note**: This will now be the new object passed to you if you add
+a `getLoadContext` function to the `createEventHandler`.


I would probably just document this in the above example, it’s not clear what ‘this’ refers to in this sentence until you inspect the types in TypeScript. And then this + the changes should be added to the Remix docs.

huw · 2022-10-13T13:31:07Z

templates/cloudflare-workers/README.md

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


waitUntil is undefined in this context. What did you mean?

Suggested change

const event = { request, env, waitUntil: context.waitUntil.bind(waitUntil) };

const event = { request, env, waitUntil: context.waitUntil.bind(context) };

huw · 2022-10-13T13:35:52Z

packages/remix-cloudflare-workers/worker.ts

+        event.respondWith(payload);
+        return;
+      }
+      return payload;


The mixed return here is tolerable (since Wrangler/Miniflare will run without typechecking), but it does mean that the fetch handler no longer conforms to ExportedHandlerFetchHandler from @cloudflare/workers-types. The code is similar enough that it would be annoying to split it into 2 separate functions, but I gave it a crack and couldn’t figure out how to use conditional types in a way that resolved it properly.

huw · 2022-10-13T13:38:20Z

packages/remix-cloudflare-workers/worker.ts

    let loadContext = getLoadContext?.(event);

    return handleRequest(event.request, loadContext);
  };
 }

 export async function handleAsset(
-  event: FetchEvent,
+  event: FetchEvent | Evt,
  build: ServerBuild,
  options?: Partial<KvAssetHandlerOptions>
 ) {


Right below this, you need to pass ASSET_NAMESPACE and ASSET_MANIFEST to getAssetFromKV. Otherwise Cloudflare won’t be able to bind the KV to the worker.

dario-piotrowicz · 2022-11-21T18:51:29Z

templates/cloudflare-workers/README.md

+```js
+import { createEventHandler } from "@remix-run/cloudflare-workers";
+
+import * as build from "../build";


shouldn't this be:

Suggested change

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

import * as build from "@remix-run/dev/server-build";

?

I think '../build' is actually fine and I misunderstood the code here, sorry, my bad 😅

ekosz · 2022-11-21T19:53:41Z

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 · 2022-11-21T21:56:49Z

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 #4198 (and there seem to even be a third one #4401 😨).

I'll be checking out #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 · 2022-11-21T22:00:22Z

@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 · 2023-05-01T22:52:34Z

@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 · 2023-05-01T22:57:15Z

@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 · 2023-05-01T23:38:11Z

@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 · 2023-05-02T00:37:57Z

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 · 2023-05-02T01:01:46Z

@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 · 2023-05-02T21:41:03Z

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 · 2024-05-22T19:37:58Z

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

remix-cla-bot bot added the CLA Signed label Dec 23, 2021

ekosz force-pushed the esm-cloudflare-workers branch from 1372f08 to 7646d75 Compare December 23, 2021 00:22

ryanflorence assigned jacob-ebey Feb 11, 2022

machour added the adapter:cloudflare-workers label Mar 17, 2022

MichaelDeBoey added the template:cloudflare-workers label Jun 2, 2022

MichaelDeBoey changed the title ~~ES Module Cloudflare Workers~~ feat(remix-cloudflare-workers): support ES Module Jun 2, 2022

MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 3, 2022

ekosz force-pushed the esm-cloudflare-workers branch from 7646d75 to f0b11db Compare September 26, 2022 04:35

ekosz added 4 commits September 26, 2022 13:38

feat(remix-cloudflare-workers): Upgrade kv-asset-handler dep

72e2e03

The newer version of the library now supports ES Modules. See: https://github.com/cloudflare/kv-asset-handler/releases/tag/v0.2.0

feat(create-remix): Update cloudflare worker README

d71f285

This adds a section explaining how to make the worker work in ESM mode.

chore: Add name to contributors.yml

91cf18a

ekosz force-pushed the esm-cloudflare-workers branch from f0b11db to 91cf18a Compare September 26, 2022 04:38

chore: Add changeset

c905446

MichaelDeBoey mentioned this pull request Oct 13, 2022

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

Closed

2 tasks

huw reviewed Oct 13, 2022

View reviewed changes

chaance removed the needs-response We need a response from the original author about this issue/PR label Oct 28, 2022

dario-piotrowicz reviewed Nov 21, 2022

View reviewed changes

huw mentioned this pull request Nov 23, 2022

feat(adapters): add cloudflare-workers-esm adapter #4676

Closed

2 tasks

buzinas mentioned this pull request Mar 4, 2023

feat(remix-dev): allow bundler as moduleResolution in tsconfig.json #5576

Merged

2 tasks

MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023

github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 1, 2023

MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023

github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 5, 2023

ryanflorence removed the adapter:cloudflare-workers label Oct 31, 2023

brookslybrand closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ekosz commented Dec 23, 2021 •

edited

Loading

remix-cla-bot bot commented Dec 23, 2021

remix-cla-bot bot commented Dec 23, 2021

ekosz commented Jan 13, 2022

ryanflorence commented Feb 11, 2022

styxlab commented Apr 25, 2022

MichaelDeBoey commented Jun 3, 2022

changeset-bot bot commented Sep 26, 2022 •

edited

Loading

ekosz commented Sep 26, 2022

MichaelDeBoey commented Sep 26, 2022

ekosz commented Sep 26, 2022

huw left a comment

huw Oct 13, 2022

huw Oct 13, 2022

huw Oct 13, 2022

petebacondarwin Nov 21, 2022

huw Oct 13, 2022

huw Oct 13, 2022

dario-piotrowicz Nov 21, 2022

dario-piotrowicz Nov 21, 2022 •

edited

Loading

ekosz commented Nov 21, 2022

dario-piotrowicz commented Nov 21, 2022

dario-piotrowicz commented Nov 21, 2022

MichaelDeBoey commented May 1, 2023 •

edited

Loading

huw commented May 1, 2023 •

edited

Loading

MichaelDeBoey commented May 1, 2023

huw commented May 2, 2023

MichaelDeBoey commented May 2, 2023

xanderberkein commented May 2, 2023 •

edited

Loading

brookslybrand commented May 22, 2024

	const event = { request, env, waitUntil: context.waitUntil.bind(waitUntil) };
	const event = { request, env, waitUntil: context.waitUntil.bind(context) };

	import * as build from "../build";
	import * as build from "@remix-run/dev/server-build";

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

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

Conversation

ekosz commented Dec 23, 2021 • edited Loading

remix-cla-bot bot commented Dec 23, 2021

remix-cla-bot bot commented Dec 23, 2021

ekosz commented Jan 13, 2022

ryanflorence commented Feb 11, 2022

styxlab commented Apr 25, 2022

MichaelDeBoey commented Jun 3, 2022

changeset-bot bot commented Sep 26, 2022 • edited Loading

🦋 Changeset detected

ekosz commented Sep 26, 2022

MichaelDeBoey commented Sep 26, 2022

ekosz commented Sep 26, 2022

huw left a comment

Choose a reason for hiding this comment

huw Oct 13, 2022

Choose a reason for hiding this comment

huw Oct 13, 2022

Choose a reason for hiding this comment

huw Oct 13, 2022

Choose a reason for hiding this comment

petebacondarwin Nov 21, 2022

Choose a reason for hiding this comment

huw Oct 13, 2022

Choose a reason for hiding this comment

huw Oct 13, 2022

Choose a reason for hiding this comment

dario-piotrowicz Nov 21, 2022

Choose a reason for hiding this comment

dario-piotrowicz Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

ekosz commented Nov 21, 2022

dario-piotrowicz commented Nov 21, 2022

dario-piotrowicz commented Nov 21, 2022

MichaelDeBoey commented May 1, 2023 • edited Loading

huw commented May 1, 2023 • edited Loading

MichaelDeBoey commented May 1, 2023

huw commented May 2, 2023

MichaelDeBoey commented May 2, 2023

xanderberkein commented May 2, 2023 • edited Loading

brookslybrand commented May 22, 2024

ekosz commented Dec 23, 2021 •

edited

Loading

changeset-bot bot commented Sep 26, 2022 •

edited

Loading

dario-piotrowicz Nov 21, 2022 •

edited

Loading

MichaelDeBoey commented May 1, 2023 •

edited

Loading

huw commented May 1, 2023 •

edited

Loading

xanderberkein commented May 2, 2023 •

edited

Loading