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.

Sign up for GitHub

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

Jump to bottom

fix: transform island components on Windows #116

Merged
merged 5 commits into from
Mar 13, 2024
Merged

fix: transform island components on Windows #116

merged 5 commits into from
Mar 13, 2024

Conversation

mrtska
Copy link
Contributor

@mrtska mrtska commented Mar 12, 2024 •
edited
Loading

This PR resolves island components doesn't transform on Windows.

On Windows, island components doesn't transform html correctly.

Input:

import { createRoute } from 'honox/factory'
import Counter from '../../islands/counter'

export default createRoute((c) => {
  return c.render(
    <div>
      <h1>Hello</h1>
      <Counter />
    </div>
  )
})

Output:

<!DOCTYPE html><html lang="en"><head><meta charset="utf-8"/><meta name="viewport" content="width=device-width, initial-scale=1.0"/><title></title><script type="module" async="" src="/static/client-UlfpEE_A.js"></script></head><body><div><h1>Hello</h1><div><p>0</p><button>Increment</button></div></div></body></html>

Details:

const defaultIsIsland: IsIsland = (id) => {
  const islandDirectoryPath = path.join(root, 'app/islands')
  return id.startsWith(islandDirectoryPath)
}

id and root are delimited by '/' on all platforms.
path.join is normalizes to the appropriate file path delimiter per platform.

On Windows, path.join returns path delimited by '\'.
id.startsWith will always false because it compares forward slash delimited paths and back slash delimited paths.

This PR adds path delimiter conversion before comparison.

@yusukebe
Copy link
Member

HI @mrtska !

Thanks for the PR! Looks good to me.

To test this code in CI, I've added the Windows CI in #117 and merged it into main. So, could you merge the main and push it?

@mrtska mrtska force-pushed the fix/island-components-on-windows branch from 0c4a492 to dba7313 Compare March 13, 2024 09:23
@mrtska
Copy link
Contributor Author

mrtska commented Mar 13, 2024

Hi, @yusukebe !

It seems unexpected bun command execution.
https://github.com/mrtska/honox/blob/d7439a8d12243593c44a6b8a442260afa7e47240/test/hono-jsx/playwright.config.ts#L20

Any ideas?🥲

@yusukebe
Copy link
Member

Hi @mrtska

Added #118. This may fix the issue, maybe!

@yusukebe
Copy link
Member

Oops. Just a moment please!

@yusukebe
Copy link
Member

@mrtska

Passed! Thanks for your contribution. Merge now!

@yusukebe yusukebe merged commit 2d73337 into honojs:main Mar 13, 2024
2 checks passed
@mrtska
Copy link
Contributor Author

mrtska commented Mar 13, 2024

Your welcome!

Thank you for your quick response😊.

Arigatou gozaimashita!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrtska @yusukebe