add ClientOnly component

[usk94]

resolves #1

I made ClientOnly component according to the https://vike.dev/client-only-components.
I use React.lazy and Suspense to display fallback component until target component is done loading.

Example of usage

  <ClientOnly
    component={() => import('../star-wars/index/+Page')}
    componentProps={{
      movies: [
        {
          id: '1',
          title: 'foo',
          release_date: 'today'
        }
      ]
    }}
    fallback={<Loading />}
  />

[usk94]

Hi, @brillout! This is the PR for adding ClientOnly Component.
Is this the right direction?

Also, since I am new to contributing to OSS, I would like to know if there are any deficiencies regarding the creation of PR.

[usk94]

If I've misunderstood the scope of the issue and it involves more extensive work, or if it's an issue that I should not be working on, I would appreciate it if you could let me know.

[brillout]

Very neat 💯 @AurelienLourot WDYT?

@usk94 Welcome to OSS contributing 🦾

[lourot]

Looks great @usk94, thanks a lot! If you feel like it, another PR with an example would be also welcome

[lourot]

Also if you feel like doing the same for Vue and Solid, very welcome too

[brillout]

I wonder if something like the following would be possible?

<ClientOnly
  fallback={<Loading />}
  component={() => import('../star-wars/index/+Page')}
  element={(Page) =>
    <Page
      movies={[
        {
          id: '1',
          title: 'foo',
          release_date: 'today'
        }
      ]}
    />
  }
/>

Or maybe even:

<ClientOnly fallback={<Loading />}>{
  async () => {
    const Page = await import('../star-wars/index/+Page')
    return (
      <Page
        movies={[
          {
            id: '1',
            title: 'foo',
            release_date: 'today'
          }
        ]}
      />
    )
  }
}</ClientOnly>

WDYT? The thing I like about this is that it's syntactically closer to "regular React", but feel free to disagree.

[usk94]

@brillout @AurelienLourot
Thank you both for reviewing my PR!
Also, regarding the examples you provided, the first one seems closer to the React style I am more familiar with.

How about this fix? ebbcafd

This allows the ClientOnly component to be used in this way.

        <ClientOnly
          fallback={<Loading />}
          component={() => import('../star-wars/index/+Page')}
          componentRenderer={(Page) => (
            <Page
              movies={[
                {
                  id: '1',
                  title: 'foo',
                  release_date: 'today'
                }
              ]}
            />
          )}
        />

[lourot]

LGTM 👍

[brillout]

LGTM 👌 Just one little nitpick: the componentRenderer name is kind of verbose. How about this:

<ClientOnly
  fallback={<Loading />}
  component={() => import('../star-wars/index/+Page')}
>{
  (Page) => (
    <Page
      movies={[
        {
          id: '1',
          title: 'foo',
          release_date: 'today'
        }
      ]}
    />
  )
}</ClientOnly>

Thoughts? As always: feel free to disagree 🙂

[usk94]

@brillout

Thanks for the feedback! I agree that it's a bit verbose.

Personally, I prefer the option of simply renaming 'componentRenderer' to 'render'. However, I'm also open to the idea of using children as a function😀

[brillout]

I was also hesitating between the two and I also thought about the name "render" 😀

And I also had a preference for "render" at first, as I ain't a fan of passing a function as children which is awkward (although I believe it's quite common in the React world?).

But, after some thoughts, I did end up prefering children as a function because I've a slight preference for this:

<GrandGrandParent>
  <GrandParent>
    <Parent>
      <ClientOnly fallback={<Loading />} load={() => import('../star-wars/index/+Page')}>{
        (Page) => (
          <Page movies={movies}>
            <Child>
              <GrandChild>
                <Leaf />
              </GrandChild>
            </Child>
          </Page>
        )
      }</ClientOnly>
    </Parent>
  </GrandParent>
</GrandGrandParent>

Over this:

<GrandGrandParent>
  <GrandParent>
    <Parent>
      <ClientOnly
        fallback={<Loading />}
        component={() => import('../star-wars/index/+Page')}
        render={
          (Page) => (
            <Page movies={movies}>
              <Child>
                <GrandChild>
                  <Leaf />
                </GrandChild>
              </Child>
            </Page>
          )
        }
      />
    </Parent>
  </GrandParent>
</GrandGrandParent>

Thoughts?

FYI I was also hesitating of whether we should rename component to load but I couldn't decide: I guess both are fine :)

Edit: renamed component to load for the first example.

[brillout]

FYI I was also hesitating of whether we should rename component to load but I couldn't decide: I guess both are fine :)

Actually, what do you think of this? I feel like it could be slightly clearer, as it's a neat break down in two parts: the loading and then the rendering. Which is exactly what is happening: lazy-loading and conditional rendering.

[usk94]

@brillout
I completely agree with renaming the 'component' prop to 'load'!
It definitely makes the purpose of the prop clearer.

Also, I've switched to the function as children approach, so please check that as well😀

[brillout]

Neat 💯

I've made some minor changes, see commits. Feel free to object to any change I've made.

I've a question: since we pass two functions to the dependency array useEffect(..., [load, children]), wouldn't that mean that the component is re-rendered everytime? This seems to be a problem and I wonder how this can be solved 🤔

[brillout]

I've a question: since we pass two functions to the dependency array useEffect(..., [load, children]), wouldn't that mean that the component is re-rendered everytime? This seems to be a problem and I wonder how this can be solved 🤔

I think the solution is to set the dependency array to the emtpy array []. Which means that the user cannot change the component but I think that's fine.

[usk94]

@brillout
Thank you for minor changes!😆

I think the solution is to set the dependency array to the emtpy array []. Which means that the user cannot change the component but I think that's fine.

Indeed, as the component is primarily used for handling heavy data fetches, it's unlikely that the children's props will change frequently.

So I have updated it to use an empty array👌

[brillout]

children's props

Ah, good point, it isn't only the component that is blocked but also any prop passed below the <ClientOnly /> boundary.

If the user wants to pass props down through the <ClientOnly /> boundary, then I think we need a solution. How about a new prop key?: string?

It actually seems quite a frequent use case. Example:

function MapWrapper(props) {
  const { geoLocation } = props
  // We need to use a key for geoLocation changes to propagate through <ClientOnly>
  return (
    <ClientOnly
      fallback={<Loading />}
      key={JSON.stringify(geoLocation)}
      load={() => import('heavy-map-library')}
    >{
      (Map) => <Map geoLocation={geoLocation} />
    }</ClientOnly>
  )
}

I wonder if there is an alternative that doesn't require key 🤔. I guess we do need a key as long as we use useEffect().

[usk94]

@brillout
Understood, thank you🙂
How about this fix? 2ef58fd

Since key is a special property in React and using it resulted in a warning, I have renamed it to a more specific name.
Also, referring to type definition for the built-in key, I have set the type to string | number.

The rendering of the component is dynamic like this.

2023-11-24.1.44.33.mov

function Page() {
  const [count, setCount] = useState(0)
  return (
    <>
      <button onClick={() => setCount((prev) => prev + 1)}>set!</button>
      <ClientOnly fallback={<Loading />} refreshKey={count} load={() => import('../star-wars/index/+Page')}>
        {(Page) => <Page movies={[{ id: '1', title: `${count}`, release_date: 'today' }]} />}
      </ClientOnly>
    </>
  )
}

[brillout]

LGTM 🟢 Ready to merge from my side. Let me know in case you object @AurelienLourot @usk94.

I made a small change: I think letting the user directly define deps is both easier for the user to understand and easier for us to communicate in the docs (we can just link to the useEffect() docs). Speaking of which, would you be up for writing documentation for <ClientOnly>? But feel free to let us know if you prefer writing code than writing documentation.

Also, if you want, a new example /examples/client-only as suggested by Aurelien would be nice.

We can merge and create new separate PRs or continue on this PR - as you wish.

[lourot]

LGTM, nice joint effort from you two :)

[usk94]

@brillout @AurelienLourot
Thank you both.
I am interested in writing documentation, so I will give it a try by creating a separate PR.

Then, may I request to merge this PR?
And going forward, if I find any issues in the main vike repository where I feel I can make a contribution, I'm also thinking of giving them a try!⭐️

[brillout]

Merged 🎉

I can't wait for us to deliver that highly polished component to users ✨

Thanks for the constructive conversation 🦾

(FYI, in case you're curious, I also thought about having both refreshKey (I really like that name) as well as deps because keys are quite common and familiar for example for TanStack Query users. But I still ended up prefering the simplicity of only having deps. As always: contradiction & conversation welcome.)

I am interested in writing documentation, so I will give it a try by creating a separate PR.

Sounds good. I'd suggest we create a new page https://vike.dev/react/ClientOnly and document it over there.

[brillout]

And going forward, if I find any issues in the main vike repository where I feel I can make a contribution, I'm also thinking of giving them a try!⭐️

💯 See contribution-welcome 💕 (but you can also pick any other (non-)existing ticket). I also just created Other projects (related to Vike) #1286.

Looking forward to it.

[usk94]

I'll look for issues from that label. Thank you!😀