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

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

Closed
brillout opened this issue Apr 6, 2024 · 29 comments
Closed

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

brillout opened this issue Apr 6, 2024 · 29 comments

Comments

@brillout
Copy link
Member

brillout commented Apr 6, 2024

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
Copy link
Collaborator

pdanpdan commented Jun 4, 2024

Hello.
Can you please take a look at #110?
It's about this issue and #67

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

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

@brillout
Copy link
Member Author

brillout commented Jun 4, 2024

  • 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
Copy link
Collaborator

pdanpdan commented Jun 4, 2024

  • I cannot think of anything that <ClientOnly> can do better that the function
  • clientOnly does not convey that it creates a component (defineComponent, defineAsyncComponent - maybe defineClientOnlyComponent)
  • <template #fallback> - I was thinking about avoiding as much as possible to override a named slot of the wrapped component

@brillout
Copy link
Member Author

brillout commented Jun 4, 2024

  • 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
Copy link
Member Author

brillout commented Jun 4, 2024

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
Copy link
Collaborator

pdanpdan commented Jun 4, 2024

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

@pdanpdan
Copy link
Collaborator

pdanpdan commented Jun 5, 2024

  • if we allow changing the fallback slot name with parameter we lose the autocomplete
  • I tested with using a Symbol for slot name - it works but autocomplete is not available and one need to import the symbol, so not very nice to use
  • I went the route of using a descriptive name of 'client-only-fallback'

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
Copy link
Member Author

brillout commented Jun 5, 2024

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

@brillout
Copy link
Member Author

brillout commented Jun 5, 2024

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
Copy link
Collaborator

pdanpdan commented Jun 5, 2024

I cannot make it work, sorry

@brillout
Copy link
Member Author

brillout commented Jun 6, 2024

  • 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
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

For me the steps are:

  • pnpm reset
  • reload VSCode so that TS is loaded with new vike-vue
  • it should work now
    I'm almost sure there is some method to force reload TS, but Developer: Reload Window works for sure

@brillout
Copy link
Member Author

brillout commented Jun 6, 2024

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

@pdanpdan
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

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

@brillout
Copy link
Member Author

brillout commented Jun 6, 2024

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
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

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
Copy link
Member Author

brillout commented Jun 6, 2024

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

@brillout
Copy link
Member Author

brillout commented Jun 6, 2024

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
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

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
Copy link
Member Author

brillout commented Jun 6, 2024

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

@pdanpdan
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

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
Copy link
Member Author

brillout commented Jun 6, 2024

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

Sounds like a Volar quirk.

pdanpdan added a commit to pdanpdan/vike-vue that referenced this issue Jun 6, 2024
…TS support vikejs#67, vikejs#82

BREAKING CHANGE: `ClientOnly` component is removed, please use the new `clientOnly` function to create components that only load and render on client side
@pdanpdan
Copy link
Collaborator

pdanpdan commented Jun 6, 2024

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

pdanpdan added a commit to pdanpdan/vike-vue that referenced this issue Jun 6, 2024
…TS support vikejs#67, vikejs#82

BREAKING CHANGE: `ClientOnly` component is removed, please use the new `clientOnly` function to create components that only load and render on client side
pdanpdan added a commit to pdanpdan/vike-vue that referenced this issue Jun 7, 2024
…TS support vikejs#67, vikejs#82

BREAKING CHANGE: `ClientOnly` component is removed, please use the new `clientOnly` function to create components that only load and render on client side
@4350pChris
Copy link
Member

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

@pdanpdan
Copy link
Collaborator

Yes, all done from my point of view

@pdanpdan
Copy link
Collaborator

BTW, do you have any idea about this?
#110 (comment)

@4350pChris
Copy link
Member

BTW, do you have any idea about this?
#110 (comment)

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

pdanpdan added a commit to pdanpdan/vike-vue that referenced this issue Jun 14, 2024
…TS support vikejs#67, vikejs#82

BREAKING CHANGE: `ClientOnly` component is removed, please use the new `clientOnly` function to create components that only load and render on client side
4350pChris pushed a commit that referenced this issue Jun 14, 2024
* feat: replace `ClientOnly` component with `clientOnly` function with TS support #67, #82

BREAKING CHANGE: `ClientOnly` component is removed, please use the new `clientOnly` function to create components that only load and render on client side

* chore: some tests to be removed before merge

* minor

* simplify slots handling?

* always require argument to be a function returning a promise?

* fix: call alternate fallback slot function

* refactor slots definition

* swallow error when loading component and expose it as prop for fallback slot

* fix slots typing

* cleanup examples

* expose attrs to fallback slots so styles can be applied if required

* minor refactor

* let's change the docs URL after vike-react also makes the migration to a clientOnly() function

* small tweak of example - move timer to the slow loading component example

* more `clientOnly` under helpers

---------

Co-authored-by: Romuald Brillout <[email protected]>
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

3 participants