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

fix(jsx): fix nested islands output by using honox/vite/components #161

Merged
merged 14 commits into from
May 3, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Apr 29, 2024

What is this?

  • When island components are nested, the outermost components are wrapped in honox-island and the inner components are not.
  • However, in template elements for restoring children, all nested components are wrapped in honox-island.

Implementation

Context API is provided by the runtime, so we need to replace it depending on the runtime the app is using. However, we can use jsxImportSource from tsconfig.json in the following places (or honoxReactImportSource if the app wants a special override) and replace it.

https://github.com/honojs/honox/compare/main...usualoma:honox:fix-nested-islands-output?expand=1#diff-dbf4bfe9bdc2ab09701d7f99d62aa69e45740919b350c8166aebe64daa3fdd54R181-R206

In addition, before this PR, the very complicated expression assembly used to be done in "src/vite/island-components.ts", but by making it independent as "src/vite/components.tsx", it can be cleaned up and future enhancements can be made easier.

tasks

  • implement
  • add tests
  • add more tests

@usualoma usualoma force-pushed the fix-nested-islands-output branch 2 times, most recently from 6d5d5f7 to 013f1f0 Compare April 29, 2024 13:23
@yusukebe
Copy link
Member

@usualoma

It looks awesome at first glance. Making HonoXIsland is good! I'll look at the details later.

My additional thoughts is if we use useContext depending on the JSX runtime we are using at the time, we can use HasIslands on any JSX runtime, not only Hono's JSX.

https://github.com/honojs/honox/blob/main/src/server/components/has-islands.tsx

@usualoma
Copy link
Member Author

@yusukebe Thank you!

My additional thoughts is if we use useContext depending on the JSX runtime we are using at the time, we can use HasIslands on any JSX runtime, not only Hono's JSX.

Sure! It seems that runtime-independent HasIslands could also be implemented.

@yusukebe
Copy link
Member

Hi @usualoma

Looks good!

One thing. This uses tsConfig.compilerOptions?.honoxReactImportSource if the user want to override it. But I think it is not good to write our custom option to tsconfig.json. So, as you mentioned, how about using useContextModule in vite.config.ts?

export default defineConfig({
  plugins: [
    honox({
      useContextModule: 'react',
    }),
  ],
})

My rough implementation:

diff --git a/src/vite/index.ts b/src/vite/index.ts
index 415ce59..bec7546 100644
--- a/src/vite/index.ts
+++ b/src/vite/index.ts
@@ -13,6 +13,7 @@ type Options = {
   devServer?: DevServerOptions
   islandComponents?: IslandComponentsOptions
   external?: string[]
+  useContextModule?: string
 }

 export const defaultOptions: Options = {
diff --git a/src/vite/island-components.ts b/src/vite/island-components.ts
index 5496971..5203051 100644
--- a/src/vite/island-components.ts
+++ b/src/vite/island-components.ts
@@ -168,39 +168,41 @@ export const transformJsxTags = (contents: string, componentName: string) => {
 type IsIsland = (id: string) => boolean
 export type IslandComponentsOptions = {
   isIsland: IsIsland
+  useContextModule?: string
 }

 export function islandComponents(options?: IslandComponentsOptions): Plugin {
   let root = ''
-  let jsxImportSource: string | undefined
+  let useContextModule = options?.useContextModule
   return {
     name: 'transform-island-components',
     configResolved: async (config) => {
       root = config.root

-      const tsConfigPath = path.resolve(process.cwd(), 'tsconfig.json')
-      try {
-        const tsConfigRaw = await fs.readFile(tsConfigPath, 'utf8')
-        const tsConfig = parseJsonc(tsConfigRaw)
+      if (useContextModule) {
+        const tsConfigPath = path.resolve(process.cwd(), 'tsconfig.json')
+        try {
+          const tsConfigRaw = await fs.readFile(tsConfigPath, 'utf8')
+          const tsConfig = parseJsonc(tsConfigRaw)

-        jsxImportSource =
-          tsConfig.compilerOptions?.honoxReactImportSource ??
-          tsConfig.compilerOptions?.jsxImportSource
-        if (jsxImportSource === 'hono/jsx/dom') {
-          jsxImportSource = 'hono/jsx' // we should use hono/jsx instead of hono/jsx/dom
+          useContextModule = tsConfig.compilerOptions?.jsxImportSource
+          if (useContextModule === 'hono/jsx/dom') {
+            useContextModule = 'hono/jsx' // we should use hono/jsx instead of hono/jsx/dom
+          }
+        } catch (error) {
+          console.warn('Error reading tsconfig.json:', error)
         }
-      } catch (error) {
-        console.warn('Error reading tsconfig.json:', error)
       }
     },
+
     async load(id) {
       if (/\/honox\/.*?\/vite\/components\./.test(id)) {
-        if (!jsxImportSource) {
+        if (!useContextModule) {
           return
         }
         const contents = await fs.readFile(id, 'utf-8')
         return {
-          code: contents.replaceAll('hono/jsx', jsxImportSource),
+          code: contents.replaceAll('hono/jsx', useContextModule),
           map: null,
         }
       }

@usualoma
Copy link
Member Author

@yusukebe

You're right, we shouldn't add our own keys to "tsconfig.json", we should be able to specify them in honox().

useContextModule?

I have previously named it useContextModule, but in the future I would like to use isValidElement as well.
https://github.com/honojs/honox/pull/162/files#diff-301678e36b084a28211205e06d8a73be0af507fd3ff8efef222069ef574ba3b4R1

So I'm thinking of making it reactApiImportSource, meaning "source for importing React API".

Priority

Suppose I consider it as reactApiImportSource. I would like to prioritise the following.

  1. Use reactApiImportSource in honox() if specified.
  2. If reactApiImportSource is not specified, try to get "compilerOptions?.jsxImportSource" from tsconfig.json <- this is the default behaviour
  3. If "compilerOptions?.jsxImportSource" cannot be obtained from tsconfig.json, the default value 'hono/jsx' is used

What do you think?

@usualoma
Copy link
Member Author

usualoma commented May 1, 2024

@yusukebe

Hypothetically, I pushed 8fb2913, which was implemented with the above content.

In most cases, automatic configuration from tsconfig.json should be fine, but if you want to specify explicitly, you can write the following.

export default defineConfig({
  plugins: [
    honox({
      islandComponents: {
        reactApiImportSource: 'react',
      },
    }),
  ],
})

Feel free to comment if you have any requests for changes.

@usualoma usualoma force-pushed the fix-nested-islands-output branch from 9ba7c00 to 7fe3d1f Compare May 1, 2024 21:40
@usualoma usualoma force-pushed the fix-nested-islands-output branch from aa1827d to 70c9f2f Compare May 1, 2024 22:34
@yusukebe
Copy link
Member

yusukebe commented May 2, 2024

@usualoma

I totally agree with naming reactApiImportSource and the priority!

One thing: how about making the src/site/components directory and putting honox-island.tsx and index.tsx instead of src/site/components.tsx?

@usualoma
Copy link
Member Author

usualoma commented May 2, 2024

@yusukebe Thank you!

Indeed, I was also wondering where to put the files.
I have changed it in 9638879, please check.

@usualoma usualoma force-pushed the fix-nested-islands-output branch from cf7a8c6 to c727336 Compare May 2, 2024 21:45
@yusukebe
Copy link
Member

yusukebe commented May 3, 2024

@usualoma

Ahhhhh, sorry, sorry. I've typed it. /site/components should be /vite/components! Because It is used for the Vite plugin. Please can you rename it?

@usualoma usualoma force-pushed the fix-nested-islands-output branch from 7e78740 to a1342e4 Compare May 3, 2024 01:12
@usualoma
Copy link
Member Author

usualoma commented May 3, 2024

OK, I did think that site and vite were prone to typo.

@usualoma
Copy link
Member Author

usualoma commented May 3, 2024

fixed in a1342e4

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

yusukebe commented May 3, 2024

@usualoma

Thaaank! I'll merge this now.

@yusukebe yusukebe merged commit 2237cdd into honojs:main May 3, 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