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

Cloudflare Workers - Content manifest as external package #4399

Closed
Acen opened this issue Oct 21, 2022 · 6 comments
Closed

Cloudflare Workers - Content manifest as external package #4399

Acen opened this issue Oct 21, 2022 · 6 comments
Assignees

Comments

@Acen
Copy link

Acen commented Oct 21, 2022

What version of Remix are you using?

1.7.3

Steps to Reproduce

Attempt to use Cloudflare Worker's static manifest from the KV.

import manifestJSON            from '__STATIC_CONTENT_MANIFEST';

const assetManifest = JSON.parse(manifestJSON);


export default {
  async fetch(request: Request, env: any, context: ExecutionContext): Promise<Response> {
	...
	return await getAssetFromKV({
              request,
              waitUntil(promise: Promise<any>) {
                return context.waitUntil(promise);
              },
            }, {
              ASSET_NAMESPACE: env.__STATIC_CONTENT,
              ASSET_MANIFEST:  assetManifest,
              cacheControl:    {
                browserTTL: ttl,
                edgeTTL:    ttl,
              },
            });
	...
	}
}

Expected Behavior

The app builds

Actual Behavior

The app throws an error and stops the build.

You can mark the path "__STATIC_CONTENT_MANIFEST" as external to exclude it from the bundle, which will remove this error.

Building Remix app in production mode...
The path "__STATIC_CONTENT_MANIFEST" is imported in server.ts but "__STATIC_CONTENT_MANIFEST" was not found in your node_modules. Did you forget to install it?

✘ [ERROR] Could not resolve "__STATIC_CONTENT_MANIFEST"

    server.ts:8:36:
      8 │ import manifestJSON            from '__STATIC_CONTENT_MANIFEST';
        ╵                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~

  You can mark the path "__STATIC_CONTENT_MANIFEST" as external to exclude it from the bundle, which will remove this error.

@Acen
Copy link
Author

Acen commented Oct 21, 2022

The work-around I've done is to install the remix-esbuild-override package and use it as part of remix.config.js as below:

const { withEsbuildOverride } = require('remix-esbuild-override')
withEsbuildOverride((option) => {
  if (
    option.entryPoints &&
    Array.isArray(option.entryPoints) &&
    option.entryPoints.includes('./server.ts')
  ) {
    if (!option.external) {
      option.external = ['__STATIC_CONTENT_MANIFEST']
    }
  }
  return option
})

@buzinas
Copy link
Contributor

buzinas commented Oct 21, 2022

@Acen This is a patch I've been using in order to support module workers:

diff --git a/dist/compiler/plugins/serverBareModulesPlugin.js b/dist/compiler/plugins/serverBareModulesPlugin.js
index 4d13e28eab536ee13b2fc6cbd28cb9475ce21fb8..a3b4e476214cfaedbb159bc647ba07f82e55f7e3 100644
--- a/dist/compiler/plugins/serverBareModulesPlugin.js
+++ b/dist/compiler/plugins/serverBareModulesPlugin.js
@@ -31,6 +31,7 @@ var fs__default = /*#__PURE__*/_interopDefaultLegacy(fs);
 
 function serverBareModulesPlugin(remixConfig, onWarning) {
   let isDenoRuntime = remixConfig.serverBuildTarget === "deno"; // Resolve paths according to tsconfig paths property
+  let isCloudflareRuntime = ["cloudflare-pages", "cloudflare-workers"].includes(remixConfig.serverBuildTarget ?? "");
 
   let matchPath = isDenoRuntime ? undefined : index.createMatchPath(remixConfig.tsconfigPath);
 
@@ -85,12 +86,8 @@ function serverBareModulesPlugin(remixConfig, onWarning) {
           }
         }
 
-        switch (remixConfig.serverBuildTarget) {
-          // Always bundle everything for cloudflare.
-          case "cloudflare-pages":
-          case "cloudflare-workers":
-          case "deno":
-            return undefined;
+        if (isDenoRuntime || isCloudflareRuntime) {
+          return undefined;
         }
 
         for (let pattern of remixConfig.serverDependenciesToBundle) {
@@ -127,7 +124,7 @@ function getNpmPackageName(id) {
 }
 
 function isBareModuleId(id) {
-  return !id.startsWith("node:") && !id.startsWith(".") && !path.isAbsolute(id);
+  return !id.startsWith("node:") && !id.startsWith(".") && !path.isAbsolute(id) && id !== "__STATIC_CONTENT_MANIFEST";
 }
 
 function warnOnceIfEsmOnlyPackage(packageName, fullImportPath, onWarning) {
diff --git a/dist/compiler.js b/dist/compiler.js
index b1cf495c3bc4ee202d4d833911773ecd26fb6bc1..c51e86596a3e665ab4c71796a70d1a412f35bfcd 100644
--- a/dist/compiler.js
+++ b/dist/compiler.js
@@ -370,6 +370,7 @@ function createServerBuild(config, options, assetsManifestPromiseRef) {
     absWorkingDir: config.rootDirectory,
     stdin,
     entryPoints,
+    external: isCloudflareRuntime ? ["__STATIC_CONTENT_MANIFEST"] : undefined,
     outfile: config.serverBuildPath,
     write: false,
     conditions: isCloudflareRuntime ? ["worker"] : isDenoRuntime ? ["deno", "worker"] : undefined,

I just opened a PR in order to get this fixed upstream.

@huw
Copy link
Contributor

huw commented May 8, 2023

Add the following to remix.config.js:

{
  serverDependenciesToBundle: [/^(?!(__STATIC_CONTENT_MANIFEST)$).*$/u],
}

This will bundle everything except __STATIC_CONTENT_MANIFEST. It’s hacky and using an unnecessarily expensive regex, but it works correctly now.

@brophdawg11 brophdawg11 added this to v2 Aug 3, 2023
@brophdawg11 brophdawg11 moved this to Backlog in v2 Aug 3, 2023
@markdalgleish
Copy link
Member

As @huw said, this can be fixed via the serverDependenciesToBundle option. Our Cloudflare Workers template currently sets this option to [/^(?!.*\b__STATIC_CONTENT_MANIFEST\b).*$/].

This approach does result in an incorrect warning in the terminal that __STATIC_CONTENT_MANIFEST hasn't been installed. I've opened a PR to address this: #7199

@MichaelDeBoey
Copy link
Member

@markdalgleish With #7214 merged, should we also remove the regex from serverDependenciesToBundle in our v2 templates?

@markdalgleish
Copy link
Member

markdalgleish commented Aug 22, 2023

@MichaelDeBoey That regex is still needed so that esbuild doesn't throw an error trying to bundle it. That PR only silenced an incorrect warning from Remix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Closed
Development

No branches or pull requests

7 participants