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

Update cloudflare with hono/context-storage external build fix #911

Conversation

rmarscher
Copy link
Contributor

re: #882 (comment)
Applies the same fix used for handling hono/context-storage with aws builds.

Copy link

vercel bot commented Sep 27, 2024

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

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Sep 28, 2024 2:38am

Copy link

codesandbox-ci bot commented Sep 27, 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.

Comment on lines -121 to -123
if (source === 'hono/context-storage') {
return { id: source, external: true };
}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, actually this is effective for all the case. Let me see if I can fix.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

Actually, @aheissenberger was right. It was my bad.
#894 is good for waku start, but #898 was wrong.

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I added more commits, but it should be fine.
If there are still some issues, feel free to open PRs.

@dai-shi dai-shi merged commit 09289da into dai-shi:main Sep 28, 2024
28 checks passed
dai-shi added a commit that referenced this pull request Sep 28, 2024
@rmarscher
Copy link
Contributor Author

Oh interesting. Either solution seemed to work from my perspective but I am not using waku start for any of my apps. Do I need to go back to importing importHonoContextStorage from waku/unstable_hono to get the context? I'll test and find out...

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

Oh, no... I'm going the circle... Let me check it too.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

As far as I tried npm run build -- --with-aws-lambda with 08_cookies, the dist folder looks good to me.

@rmarscher
Copy link
Contributor Author

Yeah, this did not work for me unless I update back to import from unstable_hone. Then it works fine. Either is ok with me. It was nice with the version I originally committed here. I was able to bring my own hono (tested with 4.6.3).

@rmarscher
Copy link
Contributor Author

I will test AWS next. I only tested Cloudflare so far.

@rmarscher
Copy link
Contributor Author

Same issue in my aws build. getContext fails if I import from 'hono/context-storage' instead of 'waku/unstable_hono'.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

Maybe it's only --with-cloudflare issue. I'm not happy with importHonoContextStorage as a user facing api.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

Hmmm? I think I miss something. Can you try 08_cookies example?

@rmarscher
Copy link
Contributor Author

When I copy 08_cookies App and Counter components into an empty page on my app, I get a fatal error

Error - Context is not available
Stack Trace
getContext (file:///var/task/components/test/App.js:12:11)
InternalAsyncComponent (file:///var/task/components/test/App.js:25:43)

The issue is caused because the entry point ("serve js") for the deploy plugins loads getContext() from waku/unstable_hono with the latest change.

It worked better IMHO when the deploy plugins imported from hono/context-storage and let the deploy server bundle include the app's copy of hono/context-storage rather than waku's. Then my app server components received the same bundled version of hono that I supplied.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

The issue is caused because the entry point ("serve js") for the deploy plugins loads getContext() from waku/unstable_hono with the latest change.

From my investigation, importHonoContextStorage and hono/context points to the same file after the build.

Let me try it again.
Using waku main 49d1ae7, and examples/08_cookies:

pnpm -r --filter='./packages/waku' run compile
cd examples/08_cookies
node ../../packages/waku/dist/cli.js build --with-cloudflare
npx wrangler pages dev

Actually I get a different error (unexpected):

⎔ Starting local server...
✘ [ERROR] service core:user:waku-project: Uncaught Error: No such module "node:path".

But, that brings me a question, how is your setup different?

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

Actually I get a different error (unexpected):

Ah, 08_cookies have node:fs. I just removed node related code in src/entries.tsx and replaced with const items: unknown[] = [];

npx wrangler pages dev is working fine.
image

@rmarscher
Copy link
Contributor Author

You're right. It does work if I deploy from examples/08_cookies directly but I think it is a side-effect of being in the same monorepo. If you change the hono version in 08_cookies to 4.6.3, you will be able to reproduce the error.

I thought I already tested using 4.6.1 from my other apps and it didn't work, but I could try again. Even if it does work, it's probably not an ideal solution.

This fixes:

--- a/packages/waku/src/lib/plugins/vite-plugin-deploy-cloudflare.ts
+++ b/packages/waku/src/lib/plugins/vite-plugin-deploy-cloudflare.ts
@@ -17,16 +17,22 @@ import { DIST_ENTRIES_JS, DIST_PUBLIC } from '../builder/constants.js';
 const SERVE_JS = 'serve-cloudflare.js';

 const getServeJsContent = (srcEntriesFile: string) => `
-import { runner, importHono, importHonoContextStorage } from 'waku/unstable_hono';
+import { runner, importHono } from 'waku/unstable_hono';
+
+let contextStorage;
+try {
+ ({ contextStorage } = await import('hono/context-storage'));
+} catch {}

 const { Hono } = await importHono();
-const { contextStorage } = await importHonoContextStorage();

 const loadEntries = () => import('${srcEntriesFile}');
 let serveWaku;

 const app = new Hono();
-app.use(contextStorage());
+if (contextStorage) {
+  app.use(contextStorage());
+}
 app.use('*', (c, next) => serveWaku(c, next));
 app.notFound(async (c) => {
   const assetsFetcher = c.env.ASSETS;

@rmarscher
Copy link
Contributor Author

^ same patch works for aws for me too.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

I think it is a side-effect of being in the same monorepo.

I think I tried it standalone too.
Just double checking; Yes, it works without monorepo.

If you change the hono version in 08_cookies to 4.6.3, you will be able to reproduce the error.

Ah, that explains. Well, then I change my opinion. If you use a different hono version, import { getContext } from 'hono/context-storage' doesn't work, and from 'waku/unstable_hono' would be the right solution for now. Unless we make hono a peer dependency (which we decided not to), there wouldn't be other solutions. This actually makes me feel like #884 can be a better solution than #891. We should definitely revisit before v1.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

We should definitely revisit before v1.

Or, I can revert it right away, because this hasn't been released yet.

@dai-shi
Copy link
Owner

dai-shi commented Sep 28, 2024

#914
sorry for going the full circle. 😄 @rmarscher @aheissenberger

dai-shi added a commit that referenced this pull request Sep 28, 2024
at least for now. this reverts #891 and re-introduce #852 and #884.

#911 (comment)
@rmarscher
Copy link
Contributor Author

#914 sorry for going the full circle. 😄 @rmarscher @aheissenberger

I see. No problem! That works. Thank you so much.

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