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: $ prefix named file as island component #171

Merged
merged 2 commits into from
May 10, 2024

Conversation

yusukebe
Copy link
Member

@yusukebe yusukebe commented May 8, 2024

With this PR, the component file whose name has $ as a prefix, like /app/routes/dir/$counter.tsx, will be treated as an island component.

Before this PR, the filenames which are island components are below:

  1. Under /islands - /islands/conter.tsx
  2. An underscore and .island.tsx suffix - /app/routes/dir/_counter.island.tsx

From my experience using HonoX, it's sometimes easy to handle if the island component is in the same directory with the route file. So the 2 is better. But I'd make it a simpler and shorter name.

  1. $ prefix - /app/routes/dir/$counter.tsx

Why $ - this is just my taste, but I think it's easy to understand.


I'll work on #159 after merging this PR.

@yusukebe
Copy link
Member Author

yusukebe commented May 8, 2024

Hey @usualoma !

What do you think about this? If it's okay, please review it!

Copy link
Member

@usualoma usualoma left a comment

Choose a reason for hiding this comment

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

I think it is a good idea. I agree!
I have made one comment, please confirm.

*/
export function matchIslandComponentId(id: string) {
return id.match(
/(\/islands\/.+?\.tsx$)|(\/routes\/.*\_[a-zA-Z0-9[-]+\.island\.tsx$)|(\/routes.*\/\$[a-zA-Z0-9[-]+\.tsx$)/
Copy link
Member

Choose a reason for hiding this comment

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

Is it intended that there is no / under routes ? If not, I think it should be added.

Suggested change
/(\/islands\/.+?\.tsx$)|(\/routes\/.*\_[a-zA-Z0-9[-]+\.island\.tsx$)|(\/routes.*\/\$[a-zA-Z0-9[-]+\.tsx$)/
/(\/islands\/.+?\.tsx$)|(\/routes\/.*\_[a-zA-Z0-9[-]+\.island\.tsx$)|(\/routes\/.*\/\$[a-zA-Z0-9[-]+\.tsx$)/

We might also want to take this opportunity to review our existing regular expressions as well. I think I can summarize as follows

  return id.match(
    /\/islands\/.+?\.tsx$|\/routes\/(?:.*\/)?(?:\_[a-zA-Z0-9-]+\.island\.tsx$|\$[a-zA-Z0-9-]+\.tsx$)/
  )

CleanShot 2024-05-09 at 05 48 45@2x

Copy link
Member Author

@yusukebe yusukebe May 9, 2024

Choose a reason for hiding this comment

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

Is it intended that there is no / under routes ? If not, I think it should be added.

I'd like it to match /routes/$counter.tsx.

Is [ necessary?

Exactly, no! It's not necessary. It's (maybe my) fault.

  return id.match(
    /\/islands\/.+?\.tsx$|\/routes\/(?:.*\/)?(?:\_[a-zA-Z0-9-]+\.island\.tsx$|\$[a-zA-Z0-9-]+\.tsx$)/
  )

Great! This will match /routes/$counter.tsx. I'll change the regex to yours and add some tests. I'll add you as a co-author for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Co-authored-by: Taku Amano <taku@taaas.jp>
@yusukebe yusukebe requested a review from usualoma May 9, 2024 14:21
@yusukebe
Copy link
Member Author

yusukebe commented May 9, 2024

Hey @usualoma

Updated: 7ff4899

Could you review it again?

Copy link
Member

@usualoma usualoma left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@yusukebe
Copy link
Member Author

Merging!

@yusukebe yusukebe merged commit 4ab674d into main May 10, 2024
2 checks passed
@yusukebe yusukebe deleted the feat/island-filename-with-dollar-prefix branch May 10, 2024 02:06
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