honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
20.41k stars 586 forks source link

__STATIC_CONTENT_MANIFEST error when bundling with Cloudflare's wrangler. #1127

Closed JeffBue closed 10 months ago

JeffBue commented 1 year ago

When using Hono@3.2.1 for Cloudflare's Workers, when bundling with Wrangler or ESbuild I get this error when importing serveStatic from hono/cloudflare-workers

✘ [ERROR] Could not resolve "__STATIC_CONTENT_MANIFEST"

    node_modules/hono/dist/adapter/cloudflare-workers/server-static-module.js:2:21:
      2 │ import manifest 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.

1 error

Even when I mark it as external and exclude it form the bundle When running the worker I get an error stating Uncaught SyntaxError: Cannot use import statement outside a module

yusukebe commented 1 year ago

Hi @JeffBue,

Try using esbuild with this command:

esbuild --outdir=dist --external:__STATIC_CONTENT_MANIFEST --bundle ./src/index.ts

But it shouldn't throw an error when using Wrangler.

JeffBue commented 1 year ago

Even when I mark it as external and exclude it form the bundle When running the worker I get an error stating Uncaught SyntaxError: Cannot use import statement outside a module

Thank you @yusukebe for responding! As noted above

Even when I mark it as external and exclude it form the bundle When running the worker I get an error stating Uncaught SyntaxError: Cannot use import statement outside a module

Here's the command logs I get:

With no --external flag error

 ~/TCG_SITES/simple-renderer-base   main ±  npx esbuild app.js --bundle --outfile=dist/worker.js --loader:.html=text --loader:.md=text --loader:.csv=text --loader:.css=text --platform=node --watch --format=esm
✘ [ERROR] Could not resolve "__STATIC_CONTENT_MANIFEST"

    node_modules/hono/dist/adapter/cloudflare-workers/server-static-module.js:2:21:
      2 │ import manifest 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.

1 error
[watch] build finished, watching for changes...
^C

With --external flag no error

 ✘  ~/TCG_SITES/simple-renderer-base   main ±  npx esbuild app.js --bundle --outfile=dist/worker.js --loader:.html=text --loader:.md=text --loader:.csv=text --loader:.css=text --platform=node --watch --format=esm --external:__STATIC_CONTENT_MANIFEST
[watch] build finished, watching for changes...

Wrangler serving compiled worker file error

 ~/TCG_SITES/simple-renderer-base   main ±  npx wrangler dev dist/worker.js --no-bundle=true
 ⛅️ wrangler 3.0.1
------------------
wrangler dev now uses local mode by default, powered by 🔥 Miniflare and 👷 workerd.
To run an edge preview session for your Worker, use wrangler dev --remote
▲ [WARNING] Enabling Node.js compatibility mode for built-ins and globals. This is experimental and has serious tradeoffs. Please see https://github.com/ionic-team/rollup-plugin-node-polyfills/ for more details.

Your worker has access to the following bindings:
- Vars:
  - API_BASE_URL: "https://static-node-api-bjoyt36yaq-uc..."
  - SITE: "base"
⎔ Starting local server...
service core:user:renderer-base: Uncaught Error: Dynamic require of "__STATIC_CONTENT_MANIFEST" is not supported
  at core:user:renderer-base:13:11
  at core:user:renderer-base:271:48
  at core:user:renderer-base:8380:3
✘ [ERROR] MiniflareCoreError [ERR_RUNTIME_FAILURE]: The Workers runtime failed to start. There is likely additional logging output above.

Please let me know if this helps narrow the troubleshooting. Also why is it necessary to always import a static package of __STATIC_CONTENT_MANIFEST ?

yusukebe commented 1 year ago

__STATIC_CONTENT_MANIFEST is required for using serve-static.

I believe you don't need to use the esbuild command; it's okay to use only the wrangler command. Wrangler includes esbuild internally. If you want to import files like *.html or *.md, and so on, please refer to this document:

JeffBue commented 1 year ago

__STATIC_CONTENT_MANIFEST is required for using serve-static.

I believe you don't need to use the esbuild command; it's okay to use only the wrangler command. Wrangler includes esbuild internally. If you want to import files like *.html or *.md, and so on, please refer to this document:

@yusukebe Is there anyway this could be incorporated in to the package. This fix works out in so many ways, since in local development, not using wrangler to compile, there is no resource called __STATIC_CONTENT_MANIFEST, but at run time there is a global variable.

// src/adapter/cloudflare-workers/server-static-module.ts
// import manifest from "__STATIC_CONTENT_MANIFEST";
import { serveStatic } from "./serve-static.js";
var module = (options = { root: "" }) => {
  return serveStatic({
    root: options.root,
    path: options.path,
    manifest: options.manifest ? options.manifest : __STATIC_CONTENT_MANIFEST,
    rewriteRequestPath: options.rewriteRequestPath
  });
};
export {
  module as serveStatic
};

commenting out the import and just using the global.

yusukebe commented 1 year ago

@JeffBue

If you follow the approach highlighted in the code, it will throw an error on Wrangler.

Screenshot 2023-06-17 at 8 12 28

Therefore, you should handle __STATIC_CONTENT_MANIFEST with the builder/bundler that you use. If you're using esbuild, you can achieve this with the --format=esm option:

esbuild --external:__STATIC_CONTENT_MANIFEST --format=esm --bundle src/static.ts > ./dist/worker.js
wrangler dev ./dist/worker.js
ifyour commented 1 year ago

When I start this project, I get the same error:

pnpm run dev
service core:user:worker: Uncaught Error: No such module "__STATIC_CONTENT_MANIFEST".
  imported from "index.js"

update: I followed this documentation and reconfigured it, problem solved.

Code-Hex commented 11 months ago

@yusukebe I'm also facing this issue. I noticed it while working on the solution for workers site at https://github.com/honojs/vite-plugins/pull/26.

This issue arises from the fact that the configuration settings for the build set by users of the hono (such as vite or tsup) cannot affect the configuration settings of the hono itself. To address this issue, it is necessary to change it to a form where the options.manifest is received externally (users explicitly import __STATIC_CONTENT_MANIFEST and pass the manifest). (In my opinion)

For example, let's say you added:

  1. import manifest from '__STATIC_CONTENT_MANIFEST'; to src/index.ts.
  2. add import { serveStatic } from 'hono/cloudflare-workers';. In the same file.

In this case, the imports will be resolved in the order of (1), then (2).

When building at this point, you will encounter the following error message:

[vite] Error when evaluating SSR module src/worker.ts: failed to import "__STATIC_CONTENT_MANIFEST"

Next, include global.d.ts in the build. Here, we will use Vite as an example. Vite plugins can resolve this import issue by using resolveId and load. Specifically, you can write code like the following:

// Plugin for Vite
{
    name: "example/vite-plugin",
    resolveId(id, importer, options) {
        if (id === "__STATIC_CONTENT_MANIFEST") {
            return id;
        }
        return null; // ignore other cases
    },
    load(id) {
        if (id === "__STATIC_CONTENT_MANIFEST") {
            // Provide a custom code for __STATIC_CONTENT_MANIFEST
            return `export default "{}"`;
        }
        return null; 
    },
}

// global.d.ts
declare module '__STATIC_CONTENT_MANIFEST' {
  const value: string;
  export default value;
}

After this, if you build again, you will notice that the initial import issue is resolved, but now an error occurs on the hono side.

[vite] Error when evaluating SSR module src/worker.ts: failed to import "hono/cloudflare-workers"
|- Error [ERR_MODULE_NOT_FOUND]: Cannot find package '__STATIC_CONTENT_MANIFEST' imported from /Users/codehex/go/src/github.com/Code-Hex/test-worker/node_modules/.pnpm/hono@3.9.2/node_modules/hono/dist/adapter/cloudflare-workers/server-static-module.js
yusukebe commented 11 months ago

@Code-Hex

You can avoid the error by marking __STATIC_CONTENT_MANIFEST as external. Try it with this config:

import { defineConfig } from 'vite'
import devServer from '@hono/vite-dev-server'

export default defineConfig({
  build: {
    rollupOptions: {
      external: ['__STATIC_CONTENT_MANIFEST']
    }
  },
  plugins: [
    devServer({
      entry: 'src/index.tsx'
    })
  ]
})
Code-Hex commented 11 months ago

@yusukebe I've already tried that. I cannot avoid it

Same reason: https://github.com/honojs/hono/issues/1127#issuecomment-1849001490

yusukebe commented 11 months ago

@Code-Hex

It does not throw the error with that config. I'm not sure when will you have the error. Can I reproduce that?

https://github.com/honojs/hono/pull/1804 is a breaking change, so we can merge it right now.

Code-Hex commented 11 months ago

@yusukebe https://github.com/Code-Hex/hono-serve

You will get error message such as below:

npm run dev

> dev
> vite

  VITE v5.0.7  ready in 104 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help
(!) Could not auto-determine entry point from rollupOptions or html files and there are no explicit optimizeDeps.include patterns. Skipping dependency pre-bundling.
12:15:38 [vite] Error when evaluating SSR module src/index.ts: failed to import "hono/cloudflare-workers"
|- Error [ERR_MODULE_NOT_FOUND]: Cannot find package '__STATIC_CONTENT_MANIFEST' imported from /Users/codehex/go/src/github.com/Code-Hex/hono-serve/node_modules/hono/dist/adapter/cloudflare-workers/server-static-module.js
    at new NodeError (node:internal/errors:405:5)
    at packageResolve (node:internal/modules/esm/resolve:782:9)
    at moduleResolve (node:internal/modules/esm/resolve:831:20)
    at defaultResolve (node:internal/modules/esm/resolve:1036:11)
    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:251:12)
    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:140:32)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:33)
    at link (node:internal/modules/esm/module_job:75:36)

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '__STATIC_CONTENT_MANIFEST' imported from /Users/codehex/go/src/github.com/Code-Hex/hono-serve/node_modules/hono/dist/adapter/cloudflare-workers/server-static-module.js
    at new NodeError (node:internal/errors:405:5)
    at packageResolve (node:internal/modules/esm/resolve:782:9)
    at moduleResolve (node:internal/modules/esm/resolve:831:20)
    at defaultResolve (node:internal/modules/esm/resolve:1036:11)
    at DefaultModuleLoader.resolve (node:internal/modules/esm/loader:251:12)
    at DefaultModuleLoader.getModuleJob (node:internal/modules/esm/loader:140:32)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:33)
    at link (node:internal/modules/esm/module_job:75:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v20.5.1
yusukebe commented 11 months ago

@Code-Hex

Thank you! It is reproduced.

However, I didn't expect serveStatic to run on @hono/vite-dev-server. In the development phase, you can use the public directory to serve static files. Use serveStatic only in the build phase and running it with wrangler.

It's the same with Cloudflare Pages. You can refer to this template:

https://github.com/honojs/starter/tree/main/templates/cloudflare-pages

If you only build it, you won't face this issue.

Code-Hex commented 11 months ago

@yusukebe Thank you for your reply.

Yes, the build step is no problem. But I don't think we can develop an application w/o development mode.

If you only build it, you won't face this issue.

The issue here arises from the fact that the __STATIC_CONTENT_MANIFEST included in hono gets bundled during package distribution, making it something that hono users cannot control during the build process.

As I mentioned in the linked comments, I am currently working on modifying the plugin mechanism to make it compatible with Wrangler as well. If you continue to hold your position, I might have to consider pausing my work.

yusukebe commented 11 months ago

@Code-Hex

How about trying this, even though it might not be what you were expecting?

https://github.com/yusukebe/hono-serve/tree/use-public

Code-Hex commented 11 months ago

@yusukebe While it might be acceptable for my specific use case, I believe it's a bit delicate to restrict users in how they can use it if we intend to release it as an official plugin.

yusukebe commented 11 months ago

@Code-Hex

Yes, ideally, we should support it. However, I think our current status is sufficient.

This is a matter of perspective. For the development phase, if we want fast reloading, we should use the public directory rather than a workers site. Vite can handle the public directory, but workers sites works in worked, which might be slower.

The reason I created the Vite plugin @hono/vite-dev-server is to use fast features like fast reloading or HMR (though it currently doesn't support SSR). Therefore, using the public directory for development makes sense to me.

Code-Hex commented 11 months ago

@yusukebe Thanks.

I believe the main focus of this discussion, in my opinion, is whether it's feasible to achieve the preliminary stage before discussing speed.

First, it should be possible to reference the 'public' path (as specified in Wrangler) only in development mode. Perhaps we share the same understanding here.

The second and foremost point is that, in development, you cannot modify the behavior specific to Workers Site (such as __STATIC_CONTENT_MANIFEST). I've been mentioning this for a while now.

The reason for this is that hono is already bundling its own specific code.

yusukebe commented 11 months ago

Thanks for summarizing.

I've been considering the __STATIC_CONTENT_MANIFEST issue and have been thought whether we should include __STATIC_CONTENT_MANIFEST in Hono's code or make it a required option.

The popular library @cloudflare/kv-asset-handler, which is used for handling KV assets, treats __STATIC_CONTENT_MANIFEST as a required option.

import { getAssetFromKV, NotFoundError, MethodNotAllowedError } from '@cloudflare/kv-asset-handler'
import manifestJSON from '__STATIC_CONTENT_MANIFEST'
const assetManifest = JSON.parse(manifestJSON)

export default {
    async fetch(request, env, ctx) {
        if (request.url.includes('/docs')) {
            try {
                return await getAssetFromKV(
                    {
                        request,
                        waitUntil(promise) {
                            return ctx.waitUntil(promise)
                        },
                    },
                    {
                        ASSET_NAMESPACE: env.__STATIC_CONTENT,
                        ASSET_MANIFEST: assetManifest,
                    },
                )
            } catch (e) {
                if (e instanceof NotFoundError) {
                    // ...
                } else if (e instanceof MethodNotAllowedError) {
                    // ...
                } else {
                    return new Response('An unexpected error occurred', { status: 500 })
                }
            }
        } else return fetch(request)
    },
}

So, I could follow this method, but I included '__STATIC_CONTENT_MANIFEST' directly in Hono's code because I prefer not to write an import statement for convenience. I'm not sure if this decision was good or bad, but in this situation, it became necessary to make it a required option.

Anyway, your PR https://github.com/honojs/hono/pull/1804 introduces a breaking change, so we can't merge it right now. If we do, it should be when releasing v4. I think the PR is good and we can include it in v4.

Code-Hex commented 11 months ago

@yusukebe How about importing this line dynamically?

https://github.com/honojs/hono/blob/75dbd4dc229335d473b809b0f2190e3b4044b9bc/src/adapter/cloudflare-workers/server-static-module.ts#L5C23-L5C48

It seems hard because it needs to be async 😇

yusukebe commented 11 months ago

@Code-Hex

Dynamic import!

Perhap, can we do that with this code?

diff --git a/src/adapter/cloudflare-workers/serve-static.ts b/src/adapter/cloudflare-workers/serve-static.ts
index e03a473..3bff189 100644
--- a/src/adapter/cloudflare-workers/serve-static.ts
+++ b/src/adapter/cloudflare-workers/serve-static.ts
@@ -35,9 +35,8 @@ export const serveStatic = (options: ServeStaticOptions = { root: '' }): Middlew
     if (!path) return await next()

     const content = await getContentFromKVAsset(path, {
-      manifest: options.manifest,
-      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-      // @ts-ignore
+      manifest:
+        typeof options.manifest === 'function' ? await options.manifest() : options.manifest,
       namespace: options.namespace ? options.namespace : c.env ? c.env.__STATIC_CONTENT : undefined,
     })
     if (content) {
diff --git a/src/adapter/cloudflare-workers/server-static-module.ts b/src/adapter/cloudflare-workers/server-static-module.ts
index 1eed8ab..0a1050e 100644
--- a/src/adapter/cloudflare-workers/server-static-module.ts
+++ b/src/adapter/cloudflare-workers/server-static-module.ts
@@ -2,7 +2,6 @@
 // eslint-disable-next-line @typescript-eslint/ban-ts-comment
 // @ts-ignore
 // For ES module mode
-import manifest from '__STATIC_CONTENT_MANIFEST'
 import type { ServeStaticOptions } from './serve-static'
 import { serveStatic } from './serve-static'

@@ -10,7 +9,9 @@ const module = (options: ServeStaticOptions = { root: '' }) => {
   return serveStatic({
     root: options.root,
     path: options.path,
-    manifest: options.manifest ? options.manifest : manifest,
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore
+    manifest: options.manifest ? options.manifest : async () => import('__STATIC_CONTENT_MANIFEST'),
     rewriteRequestPath: options.rewriteRequestPath,
   })
 }
diff --git a/tsconfig.json b/tsconfig.json
index c686743..dd5c067 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,6 +1,7 @@
 {
   "compilerOptions": {
     "target": "ES2020",
+    "module": "ESNext",
     "declaration": true,
     "moduleResolution": "Node",
     "outDir": "./dist"
Code-Hex commented 11 months ago

@yusukebe Great your idea! I will try that on my local!

yusukebe commented 10 months ago

Resolve by #1984