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

jacob-ebey · 2022-09-15T00:10:53Z

Closes: #764

Docs
Tests

Testing Strategy:

changeset-bot · 2022-09-15T00:10:55Z

⚠️ No Changeset found

Latest commit: 6c0e0d3

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

MichaelDeBoey

@jacob-ebey Do we need these updates for the cloudflare-pages template as well?

MichaelDeBoey · 2022-09-15T09:37:00Z

templates/cloudflare-workers/README.md

+    name: 🕊 Deploy
+    steps:
+      - name: 🛑 Cancel Previous Runs
+        uses: styfle/cancel-workflow-action@0.9.1


Latest version is 0.10.0

Suggested change

uses: styfle/cancel-workflow-action@0.9.1

uses: styfle/cancel-workflow-action@0.10.0

MichaelDeBoey · 2022-09-15T09:39:16Z

templates/cloudflare-workers/app/entry.server.tsx

@@ -1,4 +1,4 @@
-import type { EntryContext } from "@remix-run/cloudflare";
+import { type EntryContext } from "@remix-run/cloudflare";


Let's keep the type at the beginning of the import to keep it consistent with the rest of the codebase

Suggested change

import { type EntryContext } from "@remix-run/cloudflare";

import type { EntryContext } from "@remix-run/cloudflare";

MichaelDeBoey · 2022-09-15T09:39:35Z

templates/cloudflare-workers/types/remix-build.d.ts

@@ -0,0 +1,9 @@
+declare module "remix-build" {
+  import { type ServerBuild } from "@remix-run/cloudflare";


Suggested change

import { type ServerBuild } from "@remix-run/cloudflare";

import type { ServerBuild } from "@remix-run/cloudflare";

MichaelDeBoey · 2022-09-15T09:40:41Z

templates/cloudflare-workers/package.json

-    "dev:miniflare": "cross-env NODE_ENV=development miniflare ./build/index.js --watch",
-    "dev": "remix build && run-p \"dev:*\"",
-    "start": "cross-env NODE_ENV=production miniflare ./build/index.js"
+    "dev": "npm run build && concurrently \"npm:dev:remix\" \"npm:dev:cloudflare\"",


We're using npm-run-all in all other templates/examples, so maybe we should use that here as well to keep it consistent?

MichaelDeBoey · 2022-09-15T09:42:47Z

templates/cloudflare-workers/package.json

    "@remix-run/cloudflare": "*",
-    "@remix-run/cloudflare-workers": "*",


Is it intentional that we're not downloading @remix-run/cloudflare-workers by default anymore?

MichaelDeBoey · 2022-09-15T09:44:22Z

templates/cloudflare-workers/remix.config.js

-  // appDirectory: "app",
-  // assetsBuildDirectory: "public/build",
-  // serverBuildPath: "build/index.js",
-  // publicPath: "/build/",


We keep this commented out for convenience in our other templates, so maybe we should keep them here as well?

MichaelDeBoey · 2022-09-15T09:46:38Z

templates/cloudflare-workers/remix.init/index.js

+const path = require("path");
+
+function main({ rootDirectory }) {
+  const examplePath = path.resolve(rootDirectory, "wrangler.example.toml");


Is it intentional to keep this file in the root?

We keep remix.init files in the same folder in our stacks.

MichaelDeBoey · 2022-09-15T09:46:55Z

templates/cloudflare-workers/remix.init/package.json

@@ -0,0 +1,5 @@
+{


We can just delete this file if we don't have any dependencies

MichaelDeBoey · 2022-09-15T09:47:19Z

templates/cloudflare-workers/tsconfig.json

@@ -1,5 +1,5 @@
 {
-  "include": ["remix.env.d.ts", "**/*.ts", "**/*.tsx"],
+  "include": ["**/*.d.ts", "**/*.ts", "**/*.tsx"],


@mcansh Does this have implications for the convert-to-javascript migration?

MichaelDeBoey · 2022-09-15T09:47:51Z

templates/cloudflare-workers/types/remix.env.d.ts

@@ -1,3 +1,2 @@
 /// <reference types="@remix-run/dev" />


Should we move this file to a types folder as well in our other templates, so this is consistent?

huw · 2022-10-13T11:55:24Z

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 · 2022-10-13T12:03:22Z

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

huw · 2022-10-13T12:12:34Z

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 · 2022-10-31T15:09:59Z

@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 · 2022-11-21T23:17:52Z

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)

dario-piotrowicz · 2022-11-22T12:07:32Z

templates/cloudflare-workers/worker/index.ts

+        {
+          request,
+          waitUntil(promise) {
+            return ctx.waitUntil(promise);
+          },
+        },


doesn't this need to be casted to FetchEvent?

Suggested change

{

request,

waitUntil(promise) {

return ctx.waitUntil(promise);

},

},

{

request,

waitUntil(promise) {

return ctx.waitUntil(promise);

},

} as FetchEvent,

locally this errors for me:

Double-check if you're on @cloudflare/kv-asset-handler@0.2.0, I had the error locally but it turned out I hadn't upgraded the package properly.

Yes, that's fixed it, sorry that's my bad, thanks 🙂

huw · 2022-11-22T12:33:55Z

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

Create a new package (@remix-run/cloudflare-workers-esm or just cloudflare-esm) with ESM-specific code (i.e. tweaked feat(remix-cloudflare-workers): support ES Module #1207 code).
Update the Remix compiler to compile a new cloudflare-esm target as ESM with the neutral platform, while still bundling browser code.
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 · 2022-11-22T12:49:48Z

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

dario-piotrowicz · 2022-11-22T16:41:27Z

templates/cloudflare-workers/tsconfig.json

    "baseUrl": ".",
    "paths": {
-      "~/*": ["./app/*"]
+      "~/*": ["./app/*"],
+      "remix-build": ["./build/index.mjs"]


should this be:

Suggested change

"remix-build": ["./build/index.mjs"]

"remix-build": ["./public/build/index.mjs"]

?

MichaelDeBoey · 2023-02-06T23:50:50Z

@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 · 2023-08-15T23:11:20Z

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

feat: update cloudflare-workers template to ESM

1803c34

remix-cla-bot bot added the CLA Signed label Sep 15, 2022

jacob-ebey added 3 commits September 14, 2022 17:20

updated to let wrangler do bundling of worker

0951ae6

remove server from remix config

07bcf65

fix linting issues

6c0e0d3

MichaelDeBoey changed the title ~~feat: update cloudflare-workers template to ESM~~ feat(templates/cloudflare-workers): update to ESM Sep 15, 2022

MichaelDeBoey added the template:cloudflare-workers label Sep 15, 2022

MichaelDeBoey requested changes Sep 15, 2022

View reviewed changes

MichaelDeBoey mentioned this pull request Sep 26, 2022

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

Closed

dario-piotrowicz reviewed Nov 22, 2022

View reviewed changes

huw mentioned this pull request Nov 23, 2022

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

Closed

2 tasks

MichaelDeBoey closed this Aug 15, 2023

MichaelDeBoey deleted the cloudflare-esm-workers-template branch August 15, 2023 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jacob-ebey commented Sep 15, 2022 •

edited by MichaelDeBoey

Loading

changeset-bot bot commented Sep 15, 2022 •

edited

Loading

MichaelDeBoey left a comment

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

MichaelDeBoey Sep 15, 2022

huw commented Oct 13, 2022

MichaelDeBoey commented Oct 13, 2022

huw commented Oct 13, 2022

vstreame commented Oct 31, 2022

dario-piotrowicz commented Nov 21, 2022

dario-piotrowicz Nov 22, 2022

huw Nov 22, 2022

dario-piotrowicz Nov 22, 2022

huw commented Nov 22, 2022

dario-piotrowicz commented Nov 22, 2022

dario-piotrowicz Nov 22, 2022

MichaelDeBoey commented Feb 6, 2023

MichaelDeBoey commented Aug 15, 2023

	uses: styfle/cancel-workflow-action@0.9.1
	uses: styfle/cancel-workflow-action@0.10.0

		@@ -1,4 +1,4 @@
		import type { EntryContext } from "@remix-run/cloudflare";
		import { type EntryContext } from "@remix-run/cloudflare";

		@@ -0,0 +1,9 @@
		declare module "remix-build" {
		import { type ServerBuild } from "@remix-run/cloudflare";

	import { type ServerBuild } from "@remix-run/cloudflare";
	import type { ServerBuild } from "@remix-run/cloudflare";

		"@remix-run/cloudflare": "*",
		"@remix-run/cloudflare-workers": "*",

	"remix-build": ["./build/index.mjs"]
	"remix-build": ["./public/build/index.mjs"]

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

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

Conversation

jacob-ebey commented Sep 15, 2022 • edited by MichaelDeBoey Loading

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

⚠️ No Changeset found

MichaelDeBoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huw commented Oct 13, 2022

MichaelDeBoey commented Oct 13, 2022

huw commented Oct 13, 2022

vstreame commented Oct 31, 2022

dario-piotrowicz commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huw commented Nov 22, 2022

dario-piotrowicz commented Nov 22, 2022

Choose a reason for hiding this comment

MichaelDeBoey commented Feb 6, 2023

MichaelDeBoey commented Aug 15, 2023

jacob-ebey commented Sep 15, 2022 •

edited by MichaelDeBoey

Loading

changeset-bot bot commented Sep 15, 2022 •

edited

Loading