toss / suspensive

Manage asynchronous operations, timing, error handling, detecting intersection of elements, and caching easily and declaratively
https://suspensive.org
MIT License
490 stars 48 forks source link

[Feature] new `<ClienOnly/>` #1158

Closed manudeli closed 1 month ago

manudeli commented 1 month ago
          > > 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.

Originally posted by @manudeli in https://github.com/toss/suspensive/issues/528#issuecomment-2185584827

manudeli commented 1 month ago

@Collection50 Could I assign you for this issue?

Before assign you, I want to get approval from other maintainers.

@gwansikk @kangju2000 @bigsaigon333 @SEOKKAMONI Check this new interface to expose

If we get approval, Let's make pull requests to do below.

  1. Expose <ClientOnly/>,
  2. Make docs for https://suspensive.org
Collection50 commented 1 month ago

@manudeli Sure!

gwansikk commented 1 month ago

Before assign you, I want to get approval from other maintainers.

I agree, I think it is ready to be released.

SEOKKAMONI commented 1 month ago

I agree. Not only does the interface itself seem useful, but it also serves as a great example of using useSyncExternalStore, providing developers with good ideas. Excellent work!

kangju2000 commented 1 month ago

If we expose this component, I'm concerned that the meaning of the fallback prop might become ambiguous.

I'm worried that users might not immediately understand that this fallback is the UI shown on the server side. What do you think about this?

kangju2000 commented 1 month ago

I agree with exposing this component!

But I think it would be better to either use a more explicit prop name or hide it.

manudeli commented 1 month ago

I'm worried that users might not immediately understand that this fallback is the UI shown on the server side. What do you think about this?

Cool feedback! Thanks! @kangju2000

So I found dictionary to check what naming "fallback" means https://www.dictionary.com/browse/fallback

an act or instance of falling back. something or someone to turn or return to, especially for help or as an alternative: example: His teaching experience would be a fallback if the business failed.

For now, We are supporting fallback prop for Suspense, ErrorBoundary, Delay already. In my opinion, naming "fallback" was used for alternative ui for children's objective

Inexplicit but Consistency: Good for me below, in my opinion

<Suspense fallback={"alternative for not success of suspending"}>
  {/* Guarantee success of suspending */}
</Suspense>

<ErrorBoundary fallback={"alternative for not no error"}>
  {/* Guarantee no error */}
</ErrorBoundary>

<Delay fallback={"alternative for not delayed"}>
  {/* Guarantee delayed */}
</Delay>

So, I thought "fallback" is good prop naming for ClientOnly (alternative for not client only environment). How do you think of this logic?

<ClientOnly fallback={"alternative for not client only environment"}>
  {/* Guarantee client only environment */}
</ClientOnly>

I thought that we can use naming "fallback" for InView component too. Not perfect. but I thought that we can use naming "fallback" like this consistently

<InView fallback={"alternative for not client only environment"}> 
  {/* Guarantee in viewport or any element */}
</InView>

By this consistency, I thought fallback could be good common naming for these kind of cases

Explicit but Inconsistency: Bad for me below, in my opinion

<Suspense suspending={"alternative for not success of suspending"}>
  {/* Guarantee success of suspending */}
</Suspense>

<ErrorBoundary withError={"alternative for not no error"}>
  {/* Guarantee no error */}
</ErrorBoundary>

<Delay delaying={"alternative for not delayed"}>
  {/* Guarantee delayed */}
</Delay>

<ClientOnly server={"alternative for not client only environment"}>
  {/* Guarantee client only environment */}
</ClientOnly>

<InView notInView={"alternative for not client only environment"}>
  {/* Guarantee in viewport or any element */}
</InView>
kangju2000 commented 1 month ago

@manudeli

Thank you for your response! I agree that maintaining consistency with other components is good. Additionally, it would be helpful for users if the prop explanations were included in the documentation.

manudeli commented 1 month ago

it would be helpful for users if the prop explanations were included in the documentation.

Cool 👍

manudeli commented 1 month ago

@bigsaigon333 re-alert you🙇

bigsaigon333 commented 1 month ago

Inexplicit but Consistency: Good for me below, in my opinion

Consistency 🥇🥇🥇

manudeli commented 1 month ago

@Collection50 Could you do this please?

Collection50 commented 1 month ago

@manudeli Sure!! 👍

manudeli commented 1 month ago

@Collection50 Could you share your plan to do this?

Collection50 commented 1 month ago

@manudeli

  1. Add index file to expose.
  2. Write docs for ClientOnly

I'll make it until 3pm!

manudeli commented 1 month ago

@manudeli

  1. Add index file to expose.
  2. Write docs for ClientOnly

I'll make it until 3pm!

Thanks! We follow convention that public api should be in src/[publicApi].tsx (because of tsup setting's entry field) So move src/components/ClientOnly.tsx -> src/ClientOnly.tsx please.