Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 12 commits into from
Closed

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

wants to merge 12 commits into from

Conversation

huw
Copy link
Contributor

@huw huw commented Nov 23, 2022

Closes: #764
Supersedes: #4401 #4198 #1207

  • Docs
  • Tests

Testing Strategy:

  • I imported the compiled packages locally into my full-stack Remix project (using npm local file path resolution), then updated my server/index.ts file to use ES Modules. I checked the compiled code to ensure it was using the new dependencies as well as the new ESM code. I then ran it in miniflare@2.11.0 and deployed to Cloudflare, where everything worked.

I wasn’t able to run the deploy test because I don’t have access to Remix’s Cypress server or their production Cloudflare account.

Additional Notes

The main blocker for past attempts at this is that @cloudflare/kv-asset-handler requires the code to import a manifest file (as a JSON blob) from a special URL (__STATIC_CONTENT_MANIFEST). We can’t let esbuild try to bundle this, but if we try to add the import to @remix-run/cloudflare-workers (in order to try and interop), then esbuild leaves a raw ES6 import in the CJS code, which will fail to compile for CJS users!

In order to keep build (and code) complexity at a minimum, I think it makes the most sense to avoid interoperability & build this package separately, as a new adapter. This is similar to how Cloudflare Pages is split up, despite using an extremely similar architecture under the hood.

There are a few more scrutinable choices I made that I want to call out, in case the Remix or Workers teams disagree:

  1. In code, I’ve preferred the naming scheme cloudflare-workers vs cloudflare-workers-esm, because this means we don’t have to edit or deprecate the existing package, and the new one has a relatively short name.
  2. In docs, I’ve preferred Cloudflare’s naming scheme of ‘Service Worker syntax’ vs ‘Module Worker syntax’, but also clarified it as ‘Module Worker syntax (ESM)’ where that makes sense, just to reduce confusion as much as possible.
  3. I didn’t add/update any tests beyond the deploy test, because cloudflare-pages didn’t.
  4. I didn’t update the --platform error to include this package, since it was never available via that flag. But it might cause some extra confusion if someone knows about the flag and tries to use it?
  5. For the same reason, I didn’t update the migration/codemods for the old Remix imports.
  6. I listed this as a minor bump in the changeset, but I couldn’t find an example of a new adapter being added in the changelog, so I’m not fully sure what the Remix team wants for this.

Cheers! I’m around as much as you need me 🍻

cc @dario-piotrowicz @petebacondarwin

@changeset-bot
Copy link

changeset-bot bot commented Nov 23, 2022

🦋 Changeset detected

Latest commit: 20b7740

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

This PR includes changesets to release 17 packages
Name Type
@remix-run/cloudflare-workers-esm Patch
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

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

@@ -0,0 +1 @@
# `@remix-run/cloudflare-workers-esm`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly not sure what to do with CHANGELOG.md in a new package, but I assume the release process solves this.

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

export default { fetch: createEventHandler({ build }) };
```
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if this is sufficient. Some users browsing the docs might want to know how to bind a Durable Object with an example. On the other hand, this is a good enough starting point and I think the community can fill it in.

"compilerOptions": {
"lib": ["ES2019"],
"target": "ES2019",
"types": ["@types/node", "@cloudflare/workers-types"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps controversial, but I moved the /// <reference /> out of worker.ts and into tsconfig.json. This is best practice according to Microsoft.

In an ideal world we’d probably just do:

declare process {
  env: {
    NODE_ENV?: string;
  }
}

}
}

export function createEventHandler<Env = unknown>({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve parameterised Env here to allow the developer to supply their own contract as necessary. Better than trying to override it.

case "cloudflare-workers-esm": {
// If we're using Workers Modules (ESM), bundle everything except the special asset manifest import
if (path !== "__STATIC_CONTENT_MANIFEST") {
return undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured this was preferable to setting external in compilerServer.ts, but let me know.

Copy link
Contributor

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing stuff @huw thanks so very much for the hard work you put into this 🙏

(you did mention that you were busy so I wasn't expecting at all such quick turnaround! 🤯)

I'm not really familiar with the codebase but I've put some comments here and there (mainly just suggesting to use const instead of let 😅), hopefully some could turn out to be useful, feel free to ignore them if they aren't valid

Thanks so very much again! 😍

scripts/deployment-test/cf-workers-esm.mjs Show resolved Hide resolved
scripts/deployment-test/cf-workers-esm.mjs Show resolved Hide resolved
scripts/deployment-test/cf-workers-esm.mjs Show resolved Hide resolved
scripts/deployment-test/cf-workers-esm.mjs Show resolved Hide resolved
scripts/deployment-test/cf-workers-esm.mjs Show resolved Hide resolved
@dario-piotrowicz
Copy link
Contributor

quick ping, is anything blocking this PR? 🙂

@machour
Copy link
Collaborator

machour commented Feb 6, 2023

@dario-piotrowicz There are a lot of conflicts to begin with.

Also, my understanding is that the team isn't keen to add more adapters under the remix-run project (too much maintenance overhead). It would be best to host this adapter in a repo outside the remix org.

We just added a new "Community adapters" section in the docs for cases like this: https://remix.run/docs/en/v1/other-api/adapter#community-adapters

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Feb 6, 2023

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

@huw
Copy link
Contributor Author

huw commented Feb 7, 2023

Got it working locally with the new properties--no need to make changes to the core Remix codebase. I'll post info tomorrow and formally close the PR then.

We'll still have to publish a separate adapter package for Cloudflare ESM, otherwise users would have to add a lot of boilerplate to their codebase. My only concern here is that users could get confused between the Remix-owned worker adapters and the community/Cloudflare-owned adapters. I would suggest that either Remix own all of them, or Cloudflare own all of them. But I work for neither org so can't really make that conversation happen beyond politely asking for it :) (cc @MichaelDeBoey @dario-piotrowicz)

@dario-piotrowicz
Copy link
Contributor

@huw thanks so much for the hard work 🙂

At Cloudflare we aren't really keen on owning adapters ourselves (if we can help it), since we support many different frameworks and owning an maintaining adapters for all of them would be very costly (most frameworks do provide their own Cloudflare adapters).

As I mentioned we might do it when other alternatives are not present and there is a high enough demand for us supporting a framework.

So based on what I just mentioned our preferred way would definitely for Remix to own the Cloudflare adapters (and that's been the case so far, so I am not sure why now the Cloudflare adapters should be externalized), we can anyways contribute and help maintaining them if need be.

If that is not possible we implement and own our own adapters (but that's not a decision for me to make).

I'd be very interested to hear what @MichaelDeBoey has to say about it on behalf of the Remix team, is there a clear need/intention to externalize the Cloudflare adapters?

@kiliman
Copy link
Collaborator

kiliman commented Feb 7, 2023

Perhaps we need a @remix-community scope in npm. Something that's not official, but endorsed by the Remix team.

@MichaelDeBoey
Copy link
Member

@huw @dario-piotrowicz I don't work for the Remix team, I only contribute & help triaging things here on GitHub

@huw
Copy link
Contributor Author

huw commented Feb 9, 2023

Code is:

For src/server/createEventHandler.ts (this is pretty well exactly the same as the code in this PR):

import moduleJSON from "__STATIC_CONTENT_MANIFEST";
import {
  getAssetFromKV,
  MethodNotAllowedError,
  NotFoundError,
  type Options as KvAssetHandlerOptions,
} from "@cloudflare/kv-asset-handler";
import {
  type AppLoadContext,
  createRequestHandler as createRemixRequestHandler,
  type ServerBuild,
} from "@remix-run/cloudflare";

type AssetManifest = Omit<KvAssetHandlerOptions["ASSET_MANIFEST"], string>;
type KvAssetHandlerEvent = Parameters<typeof getAssetFromKV>[0];
type WaitUntilCallback = KvAssetHandlerEvent["waitUntil"];
interface StaticContentEnvironment { __STATIC_CONTENT: KVNamespace<string> }

const module = JSON.parse(moduleJSON) as AssetManifest;

/**
 * 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.
 */
type GetLoadContextFunction<Environment = unknown> = (
  request: Request,
  environment: Environment,
  context: ExecutionContext
) => AppLoadContext;

/**
 * Returns a request handler for the Cloudflare runtime that serves the
 * Remix SSR response.
 */
const createRequestHandler = <Environment extends StaticContentEnvironment = StaticContentEnvironment>({
  build,
  getLoadContext,
  mode,
}: {
  build: ServerBuild;
  getLoadContext?: GetLoadContextFunction<Environment>;
  mode?: string;
}) => {
  const handleRequest = createRemixRequestHandler(build, mode);

  return (request: Request, environment: Environment, context: ExecutionContext) => {
    const loadContext = getLoadContext?.(request, environment, context);

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

const handleAsset = async (
  request: Request,
  waitUntil: WaitUntilCallback,
  assetNamespace: KVNamespace<string>,
  build: ServerBuild,
  options?: Partial<KvAssetHandlerOptions>
) => {
  try {
    const mergedOptions = {
      ASSET_NAMESPACE: assetNamespace,
      ASSET_MANIFEST: module,
      ...options,
    };

    if (process.env.NODE_ENV === "development") {
      return await getAssetFromKV(
        { request, waitUntil },
        {
          cacheControl: {
            bypassCache: true,
          },
          ...mergedOptions,
        }
      );
    }

    let cacheControl = {};
    const url = new URL(request.url);
    const assetpath = build.assets.url.split("/").slice(0, -1).join("/");
    const requestpath = url.pathname.split("/").slice(0, -1).join("/");

    if (requestpath.startsWith(assetpath)) {
      // Assets are hashed by Remix so are safe to cache in the browser
      // And they're also hashed in KV storage, so are safe to cache on the edge
      cacheControl = {
        bypassCache: false,
        edgeTTL: 31_536_000,
        browserTTL: 31_536_000,
      };
    } else {
      // Assets are not necessarily hashed in the request URL, so we cannot cache in the browser
      // But they are hashed in KV storage, so we can cache on the edge
      cacheControl = {
        bypassCache: false,
        edgeTTL: 31_536_000,
      };
    }

    return await getAssetFromKV(
      { request, waitUntil },
      {
        cacheControl,
        ...mergedOptions,
      }
    );
  } catch (error) {
    if (error instanceof MethodNotAllowedError || error instanceof NotFoundError) {
      return null;
    }

    throw error;
  }
};

const createEventHandler = <Environment extends StaticContentEnvironment = StaticContentEnvironment>({
  build,
  getLoadContext,
  mode,
}: {
  build: ServerBuild;
  getLoadContext?: GetLoadContextFunction<Environment>;
  mode?: string;
}): ExportedHandlerFetchHandler<Environment> => {
  const handleRequest = createRequestHandler({
    build,
    getLoadContext,
    mode,
  });

  const handleEvent = async (
    request: Request,
    environment: Environment & StaticContentEnvironment,
    context: ExecutionContext
  ) => {
    let response = await handleAsset(request, context.waitUntil.bind(context), environment.__STATIC_CONTENT, build);

    if (!response) {
      response = await handleRequest(request, environment, context);
    }

    return response;
  };

  return (
    request: Request,
    environment: Environment & { __STATIC_CONTENT: KVNamespace<string> },
    context: ExecutionContext
  ) => {
    try {
      return handleEvent(request, environment, context);
    } catch (error: unknown) {
      if (process.env.NODE_ENV === "development" && error instanceof Error) {
        return new Response(error.message, {
          status: 500,
        });
      }

      return new Response("Internal Error", { status: 500 });
    }
  };
};

export default createEventHandler;

For global.d.ts/manifest.d.ts:

declare module "__STATIC_CONTENT_MANIFEST" {
  const value: string;
  export default value;
}

For index.ts:

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

interface Environment {
  __STATIC_CONTENT: KVNamespace<string>;
}

const index = {
  fetch: createEventHandler<Environment>({ build })
};

export default index;

And finally, remix.config.js (in ES Module syntax, since I’m assuming the package is type: “module”, but you could equally write this in CJS if you wanted):

/**
 * @type {import('@remix-run/dev').AppConfig}
 */
const config = {
  // Bundles everything _except_ `__STATIC_CONTENT_MANIFEST`. Be careful if editing.
  serverDependenciesToBundle: [/^(?!(__STATIC_CONTENT_MANIFEST)$).*$/u],
  // Optional, but highly recommended for fitting within the Worker bundle size
  serverMinify: true,
  serverModuleFormat: "esm",
  // YMMV with setting to `node`, most likely anything but simple code won’t work with the CF node compat layer
  serverPlatform: "neutral",
  // Same as previous behaviour; using main fields like this is deprecated in favour of conditions
  serverMainFields: ["browser", "module", "main"],
  // Try the `workerd` condition first (this is new and slowly standardising), then `worker`, then `browser` (equivalent of `serverPlatform: browser` but without extra behaviour.
  serverConditions: ["workerd", "worker", "browser"],
};

export default config;

You can get this working today in an ESM repo on v1.13.0. My recommendation is that, like with the existing adapters, the createEventHandler file is managed in its own package, but it’s not that much boilerplate. I will close this PR while CF & Remix discuss ^_^

@huw huw closed this Feb 9, 2023
@petebacondarwin
Copy link

Thanks for your work on this @huw. It looks like it would be fairly straightforward to implement this adapter.
Let's push forward with discussions on how it can best be published and maintained.

@jasonkuhrt jasonkuhrt mentioned this pull request Mar 23, 2023
1 task
@xanderberkein
Copy link
Contributor

I also took a shot at creating a Cloudflare Module Worker adapter and came across this PR while trying to expose __STATIC_CONTENT_MANIFEST to Cloudflare's asset handler.

I managed to get Remix working on Cloudlare's module Workers with even less boilerplate. Take a look at my worker.ts file; there's no need to reimplement handleAsset and createRequestHandler.

I published my code as a very lightweight adapter, remix-cloudflare-module-workers, and included some additional docs on integrating Workers APIs with Remix, as there aren't many resources available on this topic yet.

Since the adapter has such a low footprint, I'm happy to maintain it until Cloudflare or Remix decides to release an official one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants