vikejs / vike-vue

🔨 Vue integration for Vike
https://vike.dev/vike-vue
MIT License
38 stars 6 forks source link

`clientOnly()` function instead of `<ClientOnly>` wrapper component #82

Closed brillout closed 3 months ago

brillout commented 5 months ago

See:

I think we can & should also deprecate the <ClientOnly> wrapper component in favor of a clientOnly() function. It's a much nicer DX.

pdanpdan commented 3 months ago

Hello. Can you please take a look at https://github.com/vikejs/vike-vue/pull/110? It's about this issue and https://github.com/vikejs/vike-vue/issues/67

It's not polished and there are decisions to be made about:

But at least it works with props, events, and slots

brillout commented 3 months ago
  • should the component be removed

Does <ClientOnly> support any use case that clientOnly() doesn't? If not then, yes, I think we should remove it and release a breaking change and bump the minor semver. We're still on 0.x.y so I think it's ok.

  • the name of the function (maybe createClientOnlyComponent)

I think clientOnly() is both succint and clear?

  • where it is exposed

So far, we export utils at vike-vue/utilName. So vike-vue/clientOnly.

Eventually merging exports to the root export vike-vue is a consideration, but we need to double check whether Rollup's code-splitting works (it should in principle, but we should be sure about it as bloating the client-side is a no-go).

  • the name of the fallback slot (maybe ssr-content)

How about <template #fallback>?

I'll have a look at it later today.

pdanpdan commented 3 months ago
brillout commented 3 months ago
  • clientOnly does not convey that it creates a componen

Unless I'm missing some Vue convention, I think it's ok.

  • <template #fallback> - I was thinking about avoiding as much as possible to override a named slot of the wrapped component

Good point. How about an option? For example:

function clientOnly(
  componentLoader: () => Component,
  { fallbackSlotName = 'fallback' }: { fallbackSlotName?: string } = {}
) {
  // ...
}
brillout commented 3 months ago

The generic can be any type, maybe we should make it more restrictive?

For example, TS won't complain with the following:

clientOnly(
  () => 12
)

Did you mean more something like T extends { new (): ComponentPublicInstance } instead of T extends Component = { new (): ComponentPublicInstance }?

pdanpdan commented 3 months ago

Honestly I was just trying to find a way to pass the type of the imported component to the result of clientOnly.

pdanpdan commented 3 months ago

About typing, I cannot make heads or tails of why it accepts something like () => 2, so if someone can bring some sanity there while still keeping autocomplete on use please help.

brillout commented 3 months ago

@4350pChris WDYT? I'll have a look at it if Chris is busy.

brillout commented 3 months ago

I don't know much about Volar, but I presume using a generic to do the trick?

function clientOnly<Component, FallbackSlotName extends string = 'fallback'>(
  componentLoader: () => Component,
  { fallbackSlotName }: { fallbackSlotName?: FallbackSlotName } = {}
) {
  // ...
}

See for example the type of slotName1 and slotName2 in this playground.

pdanpdan commented 3 months ago

I cannot make it work, sorry

brillout commented 3 months ago
  • if we allow changing the fallback slot name with parameter we lose the autocomplete

I can't make autocomplete work at all on my setup. I'm trying with VSCode + Vue plugin, am I missing something?

When my cursor hovers #client-only-fall VSCode isn't suggesting #client-only-fallback:

image

pdanpdan commented 3 months ago

For me the steps are:

brillout commented 3 months ago

Ah, it works now! Thanks. Sorry I'm a VSCode newbie :sweat_smile:

pdanpdan commented 3 months ago

No problem - I feel the same with TS magic (especially when combined with Vue)

brillout commented 3 months ago

Indeed, there doesn't seem to be any Slot type that supports a extends string generic.

Would it be possible to have clientOnly() use #fallback by default and if there is a conflict with the inner component then clientOnly() uses #client-only-fallback instead? Maybe it's a bit tricky to implement?

pdanpdan commented 3 months ago

It would be the same problem with TS - we cannot make the slot name dynamic and still expose it in typescript (at least I don;t know how)

brillout commented 3 months ago

It isn't dynamic and instead we define two static slots fallback and client-only-fallback.

brillout commented 3 months ago

Maybe it's a bit tricky to implement?

If it's too tricky then I'd say let's go for only one slot client-only-fallback. But maybe it's worth trying? I think the docs can be clear:

Use #client-only-fallback if the inner component uses #fallabck.

pdanpdan commented 3 months ago

Ah, you mean like that (define both slots) - it might work. Meanwhile if someone can understand why () => 1 satisfies T extends Component it would help :D

brillout commented 3 months ago

Looking at it as we speak :)

I think the issue is somewhere else: https://play.vuejs.org/#eNp9kE9PwzAMxb+Klcs2aRpCcJoGEqBJwAEQcMylytzSkTpR4oyiqt8dp9X+HIBb/H4v9rM7deP9YpdQLdUqmlB7hoicPNiCqiutOGp1raluvAsMHQQsDNc7nAN/e4Q7J4CQGHoog2tgIr0mmjSVicToCEqamuXROOt6gTQ9n2lanY0jZYAUjI23BaNU92ity/xEU3MJYxyVdbXYRkeSuNMEoJWR3rXF8OzzQAm8hIFkVkijr8dB45BwvtfNB5rPX/RtbLOm1UvAiGGHWh0YF6FCHvH67QlbeR9g4zbJivsf+IrR2ZQzjrbbRBuJfeIb0j4Mt66peo/rlpHifqkcNDv7wa+VnDqf9a/Vj3EvFpfDP0296n8AX0unrg==

Let me have a closer look.

brillout commented 3 months ago

Btw. I think you can remove = { new (): ComponentPublicInstance }, since source is a required parameter.

brillout commented 3 months ago

Actually, () => 1 is a valid Component: https://play.vuejs.org/#eNp9kE9PwzAMxb+Klcs2aRpCcJq2SYAmAQdAwDGXKnNLR+pEiTOGqn53nFb7cwBu8fu92M9u1Y33s11CNVeLaELtGSJy8mALqpZacdRqpaluvAsMLQQsDNc7nAJ/e4Q7J4CQGDoog2tgJL1GmjSVicToCEoam/nJOGk7gTQeT2C5gsuJpsXFMFjGSMHYeFswSnWP1rrMzzQ1lUjGUVlXs210JLlbTQBaGZlQWwzPPo+V2HPoSWaFNPp67DUOCacH3Xyg+fxF38Z91rR6CRgx7FCrI+MiVMgDXr894V7eR9i4TbLi/ge+YnQ25YyD7TbRRmKf+fq0D/3Fa6re43rPSPGwVA6anV3v10oOno/71+qnuFez6/6fpk51P/UzqLo=

Which makes sense from a JSX perspective.

pdanpdan commented 3 months ago

Btw. I think you can remove = { new (): ComponentPublicInstance }, since source is a required parameter.

For some reason if I remove that we lose autocomplete for props/events/slots for resulting component

Actually, () => 1 is a valid Component

Nice - you're right

brillout commented 3 months ago

For some reason if I remove that we lose autocomplete for props/events/slots for resulting component

Sounds like a Volar quirk.

pdanpdan commented 3 months ago

Yep, editor quirk. I updated the PR - it has 2 commits - the second one should be removed before merge

4350pChris commented 3 months ago

@pdanpdan Can I merge this? Looks good from my point of view.

pdanpdan commented 3 months ago

Yes, all done from my point of view

pdanpdan commented 3 months ago

BTW, do you have any idea about this? https://github.com/vikejs/vike-vue/pull/110#discussion_r1630722333

4350pChris commented 3 months ago

BTW, do you have any idea about this? https://github.com/vikejs/vike-vue/pull/110#discussion_r1630722333

Unfortunately no. I tried a bit and couldn't get it working either.