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: Add hono enhancer to cloudflare #923

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmarscher
Copy link
Contributor

Update unstable_honoEnhancer to accept an async function to support dynamic imports.

Update unstable_honoEnhancer to pass in the Hono app and the serveWaku Hono middleware handler.

Load waku config and invoke unstable_honoEnhancer in cloudflare build. If this looks good, we should all this too all of the deploy options.

With these changes, I am able to update my waku.config.ts to this in my Cloudflare Pages project:

import type { Hono, MiddlewareHandler } from "hono";
import type { Config, unstable_HonoEnhancer } from "waku/config";

const addApiRoutes = (app: Hono) => {
  app.get("/api/hello", (c) => {
    return c.json({ hello: "world" });
  });
  return app;
};

// @ts-expect-error TODO figure out the right way to cast types here
const wakuConfig: Config = {
  ...(import.meta.env && !import.meta.env.PROD
    ? {
        unstable_honoEnhancer: async (createApp) => {
          const devServer = (await import("./waku.cloudflare-dev-server")).cloudflareDevServer({
            // Optional config settings for the Cloudflare dev server (wrangler proxy)
            // https://developers.cloudflare.com/workers/wrangler/api/#parameters-1
            persist: {
              path: ".wrangler/state/v3",
            },
          });
          return (app: Hono) => {
            // @ts-ignore ignoring types here for now
            return createApp(addApiRoutes(app));
          }
        },
      }
    : {
      // @ts-ignore ignoring types here for now
      unstable_honoEnhancer: (createApp) => (app) => createApp(addApiRoutes(app)),
    }),
  middleware: () => {
    return [
      import("waku/middleware/dev-server"),
      import("waku/middleware/headers"),
      import("waku/middleware/ssr"),
      import("waku/middleware/rsc"),
    ];
  },
};

export default wakuConfig;

I use waku dev for local development and my API route is accessible.

I use NODE_ENV=production npm run build -- --with-cloudflare to build.

Unfortunately... when I deploy to cloudflare, I am getting a strange error when trying to access the API route:

Cannot access 'loadConfig' before initialization

for the line with const configPromise = loadEntries().then((entries) => entries.loadConfig());

It works without error when I run the build using npx wrangler pages dev so that is strange.

Copy link

vercel bot commented Sep 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Sep 30, 2024 5:39am

Copy link

codesandbox-ci bot commented Sep 30, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@rmarscher rmarscher changed the title Feat/hono enhancer/cloudflare feat: Add hono enhancer to cloudflare Sep 30, 2024
@@ -49,14 +49,16 @@ export interface Config {
*/
middleware?: () => Promise<{ default: Middleware }>[];
/**
* Enhander for Hono
* Enhancer for Hono
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks.

}

export type unstable_HonoEnhancer<Hono = never, MiddlewareHandler = never> = (
Copy link
Owner

Choose a reason for hiding this comment

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

I would like to avoid exporting type from this file. If you need it in the plugin, simply define it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds good.

}

export type unstable_HonoEnhancer<Hono = never, MiddlewareHandler = never> = (
createApp: (app: Hono) => Hono,
) => Promise<(app: Hono, serveWaku: MiddlewareHandler) => Hono>;
Copy link
Owner

@dai-shi dai-shi Sep 30, 2024

Choose a reason for hiding this comment

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

Promise<> is fine, but exposing serveWaku is too much. It doesn't look like a nice enhancer abstraction.

Copy link
Owner

Choose a reason for hiding this comment

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

Another idea would be:

(createApp: (app: Hono) => Promise<Hono>) => (app: Hono) => Promise<Hono>

Copy link
Owner

Choose a reason for hiding this comment

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

accept an async function to support dynamic imports.

Hm, on second thought, I wonder if promise is really necessary. Do you really need dynamic imports?

Can you try top-level await?

At this point, it's not convincing to change unstable_honoEnhancer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you try top-level await?

Yes, I can try. It needs to import for the waku dev command and not import during build. I'm not sure if there is a better way to do that than using import.meta.env.PROD.

exposing serveWaku is too much. It doesn't look like a nice enhancer abstraction.

Exposing the built Hono middleware (serveWaku) is useful if the user doesn't want Waku to be the default route. I agree that it doesn't look as clean. Maybe we should leave it out until someone has a more compelling use case? I am fine with transforming the build output for my personal use cases.

I will make adjustments to this PR to remove the API changes.

I will also try to fix the error it throws when deployed to Cloudflare. I tried using top-level await there but the same error occurred.

Copy link
Owner

@dai-shi dai-shi Sep 30, 2024

Choose a reason for hiding this comment

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

useful if the user doesn't want Waku to be the default route.

We expose runner for such use cases. I don't think they would use unstable_honoEnhancer in such cases. But, maybe I miss some use cases.

I agree that it doesn't look as clean.

I would like it to be clean. 😄

Yes, I can try.

Let me know if it doesn't work. There might be other workarounds.

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

Successfully merging this pull request may close these issues.

2 participants