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: Script and HasIsland available at multiple runtimes with AsyncLocalStorage #168

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented May 3, 2024

#167

This appears to be a good and simple solution to implement.
However (I am not familiar with Cloudflare Workers, so maybe there is another solution) it appears that compatibility_flags = [ "nodejs_compat"] needs to be added to wrangler.toml.

@yusukebe
Copy link
Member

yusukebe commented May 3, 2024

AsyncLocalStorage! We will be able to use AsyncLocalStorage in other places, so it's not bad to use it.

compatibility_flags = [ "nodejs_compat"]

Yes, we have to set it. But there is a difference between Cloudflare Workers and Cloudflare Pages. We intent HonoX works on Pages, I'll check whether that setting works or not in Pages.

@yusukebe
Copy link
Member

yusukebe commented May 4, 2024

Hmm. It works well in local development. For production. When we use Cloudflare Workers, we write compatibility_flags on wrangler. toml. But currently, if we want to use Pages, we need to set it up on the Dashboard. This means if the user runs the npm run deploy command, it will be uploaded, but the deployment will fail the first time. This is not a good DX, though the Cloudflare team may work on it.

@yusukebe
Copy link
Member

yusukebe commented May 4, 2024

For this matter, I was thinking of providing the components by each adapter separate from honox/server like this:

import { Script } from 'honox/react/server'

Or creating @honox/react package:

import { Script } from '@honox/react/server'

With this method, we can introduce the method for createClient.

// app/client.ts
import { createClient } from 'honox/react/client'
// Or, import { createClient } from '@honox/react/client'

createClient()

Anyway, it would be best if we didn't have to make the adapter per UI library.

@yusukebe
Copy link
Member

yusukebe commented May 6, 2024

Hi @usualoma

Sorry!! I was mistaken. I asked a Cloudflare Pages team, and I tried it, and now AsyncLocalStorage works on production without setting it on the dashboard! This means a user can deploy it by only writing the flag on wrangler.toml.

Sorry for taking the trouble for you by having #170 made. Why don't we go with this approach?

@usualoma
Copy link
Member Author

usualoma commented May 8, 2024

@yusukebe

Great!!

Sorry!! I was mistaken. I asked a Cloudflare Pages team, and I tried it, and now AsyncLocalStorage works on production without setting it on the dashboard! This means a user can deploy it by only writing the flag on wrangler.toml.

#170 is part of a number of things we've tried, so don't worry about it.

If we merge this PR.

There is one point of concern. If we merge this, wrangler.toml will be required in wrangler dev and production cloudflare environment, even if the app does not use HasIsland. If existing users update, they will need to create (or maybe update) wrangler.toml after the update. Would it be possible to do it that way?

I tried "importing AsyncLocalStorage dynamically and not raising an error if import fails", but it didn't work, and even if it did work, I don't think it would be good from a DX perspective.

Benefits of being able to use AsyncLocalStorage

This would provide a "renderer-independent useRequestContext". Even if (and I don't know if this is actually the case) the user is using their own preact renderer, the renderer does not have to implement own useRequestContext.

Thus, if we decide to make honox itself (not only HasIsland) dependent on AsyncLocalStorage, we can proceed with the content of this PR. If the requirement to create (or update) wrangler.toml is a problem, we may want to consider a different approach.

@yusukebe
Copy link
Member

yusukebe commented May 9, 2024

@usualoma

There is one point of concern. If we merge this, wrangler.toml will be required in wrangler dev and production cloudflare environment, even if the app does not use HasIsland. If existing users update, they will need to create (or maybe update) wrangler.toml after the update. Would it be possible to do it that way?

I think it's not a blocker. We need just update the starter template and write about the nodejs_compat flag on README. Of course, if an existing user updates honox, their application may not work on the production. But we don't have to be so serious because HonoX is still in "alpha" status, and using AsyncLocalStorage is worth more than the pain.

Benefits of being able to use AsyncLocalStorage

Yeah. I was thinking AsyncLocalStorage would be a super powerful API for us.

Thus, if we decide to make honox itself (not only HasIsland) dependent on AsyncLocalStorage, we can proceed with the content of this PR.

Let's go with AsyncLocalStorage!

@usualoma
Copy link
Member Author

usualoma commented May 9, 2024

@yusukebe
Understood!
I think we can almost go with current PR, but give me a minute to review it once.

@usualoma usualoma force-pushed the feat/has-island-script-multi-runtime-async-local-storage branch from d23bde1 to 4d52781 Compare May 9, 2024 11:14
@usualoma usualoma marked this pull request as ready for review May 9, 2024 11:17
@usualoma
Copy link
Member Author

usualoma commented May 9, 2024

@yusukebe
Added a little test in 4d52781.
I think we can now merge this PR!

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -62,6 +63,11 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
const app = options.app ?? new Hono()
const trailingSlash = options.trailingSlash ?? false

// Share context by AsyncLocalStorage
app.use(async function ShareContext(c, next) {
Copy link
Member

Choose a reason for hiding this comment

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

This is cool! Only a few code can adapt AsyncLocalStorage to a Hono app.

@yusukebe
Copy link
Member

Hi @usualoma

Thanks! Let's go with this.

@yusukebe yusukebe merged commit 8abdc53 into honojs:main May 10, 2024
2 checks passed
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.

None yet

2 participants