search

vikejs / vike-solid

🔨 Solid integration for Vike
https://vike.dev/vike-solid
MIT License
36 stars 5 forks source link

Client-side routing breaks layouts #113

Closed jaydevelopsstuff closed 1 month ago

jaydevelopsstuff commented 2 months ago

When using client side navigation (e.g. clicking anchor links), Vike fails to add/remove Layouts from the page.

https://github.com/user-attachments/assets/b428acaa-2832-4ec7-834c-76cd61da129e

MRE Repo

  1. Install dependencies w/ pnpm
  2. Run pnpm dev and open in browser
  3. Click the links and observe that the (in this case) nested layout is not applied/removed unless the page is refreshed
brillout commented 2 months ago

@Blankeos Maybe there is an issue with the cumulative +Layout implementation?

Blankeos commented 2 months ago

Looking into this!

Blankeos commented 2 months ago

Hi @jaydevelopsstuff. Looked into your MRE, thanks for this, I could play around with the issue immediately! Found that this isn't a Vike issue. It's just a very common misuse of SolidJS if you're coming from React.

TL;DR; You're destructuring props. Don't do that. I have some tips below though. I made a PR on your repo btw for the fix.

Few tips about SolidJS and props

// ❌ bad import { FlowProps } from "solid-js" const MyComponent = ({ children }: FlowProps) => { const { children } = props; // ... }

// ✅ good (also personally useful because you know which ones are props from a glance) import { FlowProps } from "solid-js" const MyComponent = (props: FlowProps) => { // ... }

// ✅ good (if you really want to destructure) import { mergeProps, splitProps, FlowProps } from "solid-js" const MyComponent = (props: FlowProps<{ age?: number }>) => { // 1. First way: always use if you want to destructure and also add default values. // I cannot confirm if this works actually lol, so I recommend the 2nd way. const { children, age } = mergeProps({ age = 14}, props)

// 2. Second Way: or still keep it inside a parent object (I like this personally because of the same reason above) const _props = mergeProps({ age = 14}, props)

// 3. Third way: Similar const { children, age, ...restProps } = props; const [otherProps, restProps] = splitProps(props, ["age"]); // otherProps has age. restProps has children, and etc.

// ... }



Also some improvements, I could add in your code (this doesn't matter though, I just realized):
- [File Structure](https://vike.dev/file-structure#basic). Your `+Page.tsx` that lives in the `/` route shouldn't be in `src/pages/+Page.tsx` (I guess that's one difference from Next.js and SvelteKit). But apparently, it still works without putting it inside `index/+Page.tsx` (I didn't know that @brillout lol).

    I think this is also good to do for you since you might want to add `+config.ts` inside the `/` route only. Or a `+title.ts` (to change the page title).

<img width="763" alt="image" src="https://github.com/user-attachments/assets/0745d45e-dec5-49bb-ab52-4f21ee25bd7c">

---

By the way if this helped you, I'll just plug my SolidJS + Vike boilerplates here:
- [solid-hop](https://github.com/blankeos/solid-hop) - simpler (like a `create-next` `create-solid` `create-svelte`) barebones starter for SolidJS.
- [solid-launch](https://github.com/blankeos/solid-launch) - fully-fledged with auth, folder structures, conventions, snippets, etc.
brillout commented 2 months ago

@Blankeos Great answer :100: and makes sense about deconstructing props. Although I ain't sure I understand the issue about src/pages/+Page.tsx; do you mean that it should be defined in src/pages/index/+Page.tsx instead? Why is that? :eyes:

By the way if this helped you, I'll just plug my SolidJS + Vike boilerplates here:

  • solid-hop - simpler (like a create-next create-solid create-svelte) barebones starter for SolidJS.
  • solid-launch - fully-fledged with auth, folder structures, conventions, snippets, etc.

These look great! We could add them to vike-solid's readme/docs if you want. (Btw. we're thinking of adding Kysely to Bati.)

Also, if you're up for it, you could co-lead vike-solid with us (and there is an opportunity to completely lead vike-solid as we are all fairly busy with Vike core and Bati.)

jaydevelopsstuff commented 1 month ago

Thanks so much for the help, just a dumb mistake.

Blankeos commented 1 month ago

Happy to help @jaydevelopsstuff. It's perfectly okay to make that mistake. It's just the React-to-Solid jet lag haha. I made the same mistake before lol.

Also, hi @brillout, I think I missed reading your reply haha.

do you mean that it should be defined in src/pages/index/+Page.tsx instead? Why is that? 👀

Yup, based on the file structure docs, I never really thought src/pages/+Page.tsx was possible haha. But after reading it again, yeah it actually is there src/pages/**/+Page.js. That's cool!

Will probably still stick with src/pages/index/+Page.tsx though because +data.ts or +config.ts can be placed there if needed.

These look great! We could add them to vike-solid's readme/docs if you want.

Really appreciate that coming from you! Sounds awesome! Yeah, I think it would help the Vike + Solid community a lot if it's on the examples.

Also, if you're up for it, you could co-lead vike-solid with us

It would be a really cool opportunity, but not sure if I'm ready for the obligation. What does the co-lead usually do? Feel free to dm me on Discord!

In any case, I'll do my best answering questions in the community. I think it helps everyone and the framework in the long term. Would really love to see Vike becoming mainstream with giants like NextJS and upcoming ones like Tanstack Router.

brillout commented 1 month ago

It would be a really cool opportunity, but not sure if I'm ready for the obligation. What does the co-lead usually do?

You can contribute however you want and feel like.

Examples: