vikejs / vike-vue

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

feat: replace ClientOnly component with clientOnly function #82 #110

Closed pdanpdan closed 2 weeks ago

pdanpdan commented 3 weeks ago

Improves DX

pdanpdan commented 3 weeks ago

This should be now ready. Please do not merge before we remove the second commit - it is just to have a playground while testing

brillout commented 3 weeks ago

I made some substantial changes, let me know what you think! See commit messages.

pdanpdan commented 3 weeks ago

Fixed one little glitch, the rest is ok with me. We need to decide what to keep in the examples - should I clean up examples to original and just replace ClientOnly usage with clienOnly?

brillout commented 3 weeks ago

Ah, thanks for the fix.

Also, have you seen the comment here and there?

About the example, yea we should make it easier for the user to digest. We can showcase only one, or more than one interesting use case. It's /examples/full after all :)

brillout commented 3 weeks ago

Btw. I didn't test my changes, so maybe it doesn't work.

pdanpdan commented 3 weeks ago

I think it's ready (including examples)

brillout commented 3 weeks ago

Very neat, clientOnly.ts looks really nice :sparkles:

There are quite a lot of examples, which I actually like :+1:. It's a dedicated page so it makes sense. It's nice for users to have a playground with lots of exampels to play with and going deep, while vike.dev can have easy docs while pointing to /examples/full/.

One last thing: the utils/ directory is for small general purpose utilities like lodash. I think we should create a new directory for clientOnly.ts. Ideas for how to name that directory? Maybe helpers/? Maybe there is a terminology in Vue for these kind of helpers?

(I made some minor polishing. As always feel free to disagree and I'm happy if vike-vue eventually adopts a different style in the future.)

pdanpdan commented 3 weeks ago

I cannot find another good category name for this kind of functions. The closest I can think of is renderer (it's a function that implements how something is rendered) / render (if we don't want to clutter renderer). wrappers might work, or as a catchall helpers

brillout commented 3 weeks ago

I cannot find another good category name for this kind of functions. The closest I can think of is renderer (it's a function that implements how something is rendered) / render (if we don't want to clutter renderer). wrappers might work, or as a catchall helpers

I think render(er) is a little too vague and ambiguous with Vike's renderer/ directory.

I've a slight preference for helpers/, but we can also go for wrappers/ if you prefer.

pdanpdan commented 3 weeks ago

I'll move it to helpers - I realized it's publicly from the root of the package so it does not matter to much

pdanpdan commented 3 weeks ago

I cannot find a way to infer the attrs type, so until someone else figures it out I would say this should be ready

brillout commented 3 weeks ago

:+1: The PR LGTM.

@4350pChris WDTY?

4350pChris commented 2 weeks ago

@pdanpdan Would you be so kind and fix the merge conflicts?

pdanpdan commented 2 weeks ago

done