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: preserved file island support #114

Merged
merged 3 commits into from
Mar 17, 2024

Conversation

yossydev
Copy link

@yossydev yossydev commented Mar 11, 2024

Overview

Files of the preserved series in this PR,

  • _renderer.tsx
  • _404.tsx
  • _error.tsx

When the client component is imported with _renderer.tsx and _404.tsx, js is loaded on the client.

Context

When I made a blog with HonoX before, I imported the island component I made in _renderer.tsx, but currently, JS was not loaded on the client even if I added it to the preserved system file.

@yossydev
Copy link
Author

yossydev commented Mar 14, 2024

@yusukebe
I am sorry that the code is still halfway through implementation, but I would like to discuss one point.

This PR is for the question I asked at X (https://x.com/yusukebe/status/1765727221526983014?s=20).
In the src/server/server.ts that I am modifying in this PR, even if there is no route and the island component is imported to _renderer.tsx, I was able to make it so that the script is called on the client.

However, the options I set for jsxRenderer like docType are no longer being applied and I am stuck without a good solution.

Any suggestions?
In the current src/server/server.ts, even if there is no route and the island component is imported to _renderer.tsx, I was able to get the script to be called on the client.

@yossydev
Copy link
Author

I understand.
If I had written the docType in the top _renderer.tsx, the docType option didn't seem to work, because with my current implementation, it is that renderer.tsx that is being checked, not the top.

@yusukebe
Copy link
Member

Okay @yossydev! Please let me know if the coding is finished. Thanks!

@yossydev yossydev force-pushed the feat/island-preserved-support branch 2 times, most recently from 3a0ac28 to c5a85fc Compare March 16, 2024 10:05
@yossydev yossydev changed the title island preserved support feat: preserved file island support Mar 16, 2024
@yossydev yossydev marked this pull request as ready for review March 16, 2024 10:12
@yossydev
Copy link
Author

@yusukebe
I feel like I could implement this and would appreciate a review!

@yossydev yossydev force-pushed the feat/island-preserved-support branch from c5a85fc to c13fc9b Compare March 16, 2024 10:35
@yusukebe
Copy link
Member

yusukebe commented Mar 16, 2024

Hi @yossydev

Thanks for the PR.

I think we can make this code short.

In this case, the value of IMPORTING_ISLANDS_ID can be just a boolean check to see if the route has islands. Also, if the renderer has islands, then the route will always have islands, so the code can be shortened.

diff --git a/src/server/server.ts b/src/server/server.ts
index 4fd73ef..a682531 100644
--- a/src/server/server.ts
+++ b/src/server/server.ts
@@ -103,18 +103,14 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
       const rendererPaths = getPaths(dir, rendererList)
       rendererPaths.map((path) => {
         const renderer = RENDERER_FILE[path]
-        const setInnerMeta = createMiddleware(async function innerMeta(c, next) {
-          // @ts-expect-error renderer[importing_islands_id] is not typed
-          const importingIslands = renderer[IMPORTING_ISLANDS_ID] as boolean
-          if (importingIslands) {
-            hasIslandComponent = true
-          }
-          c.set(IMPORTING_ISLANDS_ID as any, importingIslands)
-          await next()
-        })
+        // @ts-expect-error it is not types
+        const importingIslands = renderer[IMPORTING_ISLANDS_ID] as boolean
+        if (importingIslands) {
+          hasIslandComponent = true
+        }
         const rendererDefault = renderer.default
         if (rendererDefault) {
-          subApp.all('*', rendererDefault, setInnerMeta)
+          subApp.all('*', rendererDefault)
         }
       })

@@ -136,9 +132,7 @@ export const createApp = <E extends Env>(options: BaseServerOptions<E>): Hono<E>
         // @ts-expect-error route[IMPORTING_ISLANDS_ID] is not typed
         const importingIslands = route[IMPORTING_ISLANDS_ID] as boolean
         const setInnerMeta = createMiddleware(async function innerMeta(c, next) {
-          if (!hasIslandComponent) {
-            c.set(IMPORTING_ISLANDS_ID as any, importingIslands)
-          }
+          c.set(IMPORTING_ISLANDS_ID, importingIslands ? true : hasIslandComponent)
           await next()
         })

@yossydev
Copy link
Author

@yusukebe
Thanks for the review!
Well, it can certainly be shorter than the previous code, as you said!

a1c8f19 ← Fixed it here!

@yusukebe
Copy link
Member

@yossydev

Thanks! One thing. We have to consider the name of the test application. Currently, you named the app as test/hono-jsx/app-blog. I think it's not bad, but there will be more descriptive naming. How about test/hono-jsx/app-islands-in-preserved?

@yossydev
Copy link
Author

@yusukebe
reply: #114 (comment)

Thank you for the review!!
I also thought the app name was not the best! Indeed, your suggestion seems better!

I have made the correction here: edf3f23

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!

@yusukebe
Copy link
Member

@yossydev

Thanks! Let's go with it.

@yusukebe yusukebe merged commit c97290c into honojs:main Mar 17, 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.

2 participants