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: enable to put "island" not only in islands directory #140

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

yusukebe
Copy link
Member

@yusukebe yusukebe commented Apr 10, 2024

Related to #45

With this PR, you can put an island component in a directory under /app/routes, not only the /app/islands directory. You should name it with a _ prefix, like _counter.island.tsx.

app
├── client.ts
├── global.d.ts
├── islands
│   └── counter.tsx
├── routes
│   ├── directory
│   │   ├── _counter.island.tsx // <--- You can put it here
│   │   └── index.tsx
│   ├── index.tsx
└── server.ts

It was inefficient to put Islands in /app/islands, which are used only in a certain directory. However, this PR makes it easier to understand the structure by placing Islands in the same directory as the files that use them.

@yusukebe
Copy link
Member Author

Hi @usualoma @bruceharrison1984

Could you review this?

@bruceharrison1984
Copy link
Contributor

LGTM, however why constraint it to islands and routes? Why not just make it src in general? A common pattern i use is to place components into src/components, which wouldn't be possible in this case.

Either way, I think this is a good incremental improvement to the Islands concept.

@yusukebe
Copy link
Member Author

Thanks @bruceharrison1984 !

however why constraint it to islands and routes?

Good question! This is related to the discussion in #45.

At first, I also thought I could just put the island file in /app/islands. However, for DX improvement, I sometimes wanted to place an island in the same directory for the route files.

This is one example:

.
└── app
    ├── islands
       └── online-status.tsx
    └── routes
        ├── chats
           ├── [id]
              ├── _chat.island.tsx
              └── index.tsx
           └── index.tsx
        └── index.tsx

I think it would be better to put common islands in /app/islands and specific ones in /app/routes/sub/_foo.island.tsx.

@yusukebe
Copy link
Member Author

@usualoma Any thoughts?

@usualoma
Copy link
Member

@yusukebe Thanks. LGTM!

@yusukebe
Copy link
Member Author

Thanks! Merge now.

@yusukebe yusukebe merged commit a13422d into main Apr 13, 2024
2 checks passed
@yusukebe yusukebe deleted the feat/island-not-only-islands-directory branch April 13, 2024 00:01
@liltechnomancer
Copy link

liltechnomancer commented Apr 24, 2024

Thanks @bruceharrison1984 !

however why constraint it to islands and routes?

Good question! This is related to the discussion in #45.

At first, I also thought I could just put the island file in /app/islands. However, for DX improvement, I sometimes wanted to place an island in the same directory for the route files.

This is one example:

.
└── app
    ├── islands
       └── online-status.tsx
    └── routes
        ├── chats
           ├── [id]
              ├── _chat.island.tsx
              └── index.tsx
           └── index.tsx
        └── index.tsx

I think it would be better to put common islands in /app/islands and specific ones in /app/routes/sub/_foo.island.tsx.

I think @bruceharrison1984 was asking why it needs to be in routes (or islands) and it couldn't just include any file with the _component.island.tsx naming convention in app or src.

For example app/comonents/_component.island.tsx or app/auth/_component.island.tsx

My project is ordered by features so it would be nice to do

.
└── app
    └── auth
        ├── components 
          ├── _sign-in.island.tsx
        └── index.tsx

I decided to patch this in, I made an issue that includes the patch if you are interested.
#159

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.

4 participants