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

[Feature]: Determine what is clientOnly prop of <Suspense/>, <ClientOnly/>. #528

Open
manudeli opened this issue Dec 31, 2023 · 10 comments
Open

Comments

@manudeli
Copy link
Member

manudeli commented Dec 31, 2023

Package Scope

@suspensive/react

Possible Solution

  1. Current implementation
  2. useSyncExternalStore: https://twitter.com/TkDodo/status/1741068994981826947?t=XmG17etMUL2m0JFim03vqw&s=19
  3. Global variable with initial state: https://baek2back.gitbook.io/fundamental/hooks/usehydrated

etc.

No response

@manudeli manudeli self-assigned this Dec 31, 2023
@Collection50
Copy link
Contributor

Hello, @manudeli
I am very inspired by your activities.
<ClientOnly/> looks useful, but I don't think it's been worked on yet.
Can I be assigned this issue?

@manudeli
Copy link
Member Author

@Collection50 Thank you for your interest in Suspensive. If you submit it as a Pull Request, I think we can provide it as an experimental feature.

But, There are two reasons why we haven't implemented the ClientOnly component yet.

  1. This is because the clear usage of the ClientOnly component has not yet been determined.

If you have a use case in mind, please suggest it.

  1. The naming of ClientOnly may conflict with clientOnly prop of Suspense. However, this may confuse library users because the internal implementations are different.

clientOnly in https://github.com/toss/suspensive/blob/main/packages/react/src/hooks/useIsClient.ts is handled by useIsomorphicLayoutEffect. I think it could be a different implementation than useSyncExternalStore in this issue and this could be the problem.

It would be nice to provide a unified implementation of the ClientOnly component and the clientOnly prop of Suspense in the same way.

@manudeli
Copy link
Member Author

@Collection50 Here is another implementation of ClientOnly: https://baek2back.gitbook.io/fundamental/hooks/usehydrated

  1. Current implemantation: This way use component state to check if component self mounted.
  2. Global Variable: This way use global variable as initial state of component to check if page already hydrated
  3. useSyncExternalStore: This way use React 18 hook to check server or client

@manudeli manudeli changed the title [Feature]: <ClientOnly/> with useSyncExternalStore on react 18 [Feature]: Determine what is clientOnly prop of <Suspense/>, <ClientOnly/>. Jun 24, 2024
@Collection50
Copy link
Contributor

Collection50 commented Jun 24, 2024

@manudeli
I think useSyncExternalStore is better way because it's react hook and performance is good.

As you said, I was actually thinking about where to use <ClientOnly/>.
Suspense's clientOnly is implemented as a useIsClient using useEffect, so I was wondering how to integrate it.
I'm a little confused about your intentions, is it correct to say to integrate <ClientOnly/> with Suspense's clientOnly using 1 of the 3 you said?

@manudeli
Copy link
Member Author

I think useSyncExternalStore is better way because it's react hook and performance is good.

I agree with this opinion if we can use it

is it correct to say to integrate <ClientOnly/> with Suspense's clientOnly using 1 of the 3 you said?

Yes, I think that <Suspense clientOnly/> should use internally to unify meaning of how clientOnly prop of Suspense working and how working.

So first, Could you make component with current implementation(Current implemantation: This way use component state to check if component self mounted.) internally without exposing it as public api please?

Internal api shouldn't be src/*.tsx. so make file in src/components/ClientOnly.tsx and use it in src/Suspense.tsx please first.

@manudeli
Copy link
Member Author

@gwansikk Care this issue with me please. This will be important improvement if we can

@manudeli manudeli changed the title [Feature]: Determine what is clientOnly prop of <Suspense/>, <ClientOnly/>. [Feature]: Determine what is clientOnly prop of <Suspense/>, <ClientOnly/>. Jun 24, 2024
@Collection50
Copy link
Contributor

@manudeli

Can I be assigned this issue?

As mentioned above, I wanted to work on it, is there anything I can contribute to?

@Collection50
Copy link
Contributor

Could you make component with current implementation internally without exposing it as public api please?

I would really appreciate it if you could explain your intentions more clearly.

@manudeli
Copy link
Member Author

manudeli commented Jun 24, 2024

Can I be assigned this issue?

As mentioned above, I wanted to work on it, is there anything I can contribute to?

I assigned you. Thanks for your contributing!

Could you make component with current implementation internally without exposing it as public api please?

I would really appreciate it if you could explain your intentions more clearly.

  1. Make <ClientOnly/> in src/components/ClientOnly.tsx to avoid tsup build entry.
  2. Use <ClientOnly/> in src/Suspense.tsx. but Don't expose <ClientOnly/> in index.ts yet. In my opinion, We should expose <ClientOnly/> as public api gradually. so I think this should be next step.

@Collection50
Copy link
Contributor

Could you make component with current implementation internally without exposing it as public api please?

I would really appreciate it if you could explain your intentions more clearly.

  1. Make <ClientOnly/> in src/components/ClientOnly.tsx to avoid tsup build entry.
  2. Use <ClientOnly/> in src/Suspense.tsx. but Don't expose index.ts yet. In my opinion, We should expose <ClientOnly/> as public api gradually. so I think this should be next step.

Yes I understood that don't expose interfaces that will be exposed to people using Suspective.

manudeli added a commit that referenced this issue Jun 24, 2024
related #528 

# Overview

Implemented the `ClientOnly` component.

## PR Checklist

- [x] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md)
2. I added documents and tests.

---------

Co-authored-by: Jonghyeon Ko <jonghyeon@toss.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants