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

feat: make Layout setting cumulative #86

Merged
merged 6 commits into from
Jun 18, 2024
Merged

Conversation

Blankeos
Copy link
Contributor

@Blankeos Blankeos commented Jun 13, 2024

This PR implements cumulative Layouts for Vike + Solid similar to:

Context:

I'm using these as reference:

[Edited this since blockers are resolved]

@Blankeos Blankeos mentioned this pull request Jun 13, 2024
@brillout
Copy link
Member

brillout commented Jun 14, 2024

I ain't familiar with Solid, but I wonder: would this work?

<Dynamic component={Layout1}>
   <Dynamic component={Layout2}>
     {props.children}
   </Dynamic>
</Dynamic>

Wouldn't the following do the trick then?

let el = props.children
Layouts.forEach(Layout => {
  const child = el
  el = <Dynamic component={Layout}>{child}</Dynamic>
})

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 14, 2024

@brillout yes that's what I'm trying to accomplish, that solution unfortunately doesn't work. Just tried it, it will cause hydration errors. For instance:

function Layout(props: ParentProps) {
  const pageContext = usePageContext();
  
  let el = props.children;
  pageContext.config.Layout?.forEach((Layout: FlowComponent) => {
    const child = el;
    el = <Dynamic component={Layout}>{child}</Dynamic>;
  })

  return el;
}

That would work in React, but not in SolidJS. That's technically also the reason why it's tricky because Solid usually doesn't like "early returns". Anything in Solid component's function body will only run once--Unless it's memoized or is a signal (createMemo, createStore, createSignal). A single component in SolidJS essentially only returns once. Doing stuff like this will render correctly "initially", but will lose out on reactivity. BUT if getPageElement only actually runs on the server, not on the client-side. Then this "maintaining reactivity" argument isn't really important because both just spit out the same HTML "initially".

I tried a couple other solutions (this current PR using recursion), but no luck. This actually does maintain reactivity I think (based on the other SolidJS libraries using that pattern, I linked them above).

But here's an approach that didn't cause hydration errors though:

function Layout(props: ParentProps) {
  const pageContext = usePageContext();

  return (
    <For each={pageContext.config.Layout?.reverse() ?? []}>
      {(Layout: FlowComponent) => (
        <Dynamic component={Layout}>{props.children}</Dynamic>
      )}
    </For>
  );
}

Weirdly, the client-side version likes this even if the component tree is ultimately looking like:

<Layout1>
  {props.children}
</Layout2>
<Layout2>
  {props.children}
</Layout2>

So, it actually got me thinking... Is getPageElement only running on the server?

To confirm this, on the client-side app, I made the +config to ssr: false, then removed the <Layout> usage on getPageElement in vike-solid. The client-side does still actually try to render the layouts in the same weird way as above (NOTE: even when I removed the Layout on getPageElement already). Maybe there's something wrong with how the client-side renders it and causes these hydration errors?

Notice there's no hydration errors, there are layouts Even though I removed the on the getPageElement
image image

Can you point me into the right direction for investigating this behavior?

To replicate this so far:

Reqs: Bun, PNPM

  1. On vike-solid: pnpm build then bun link
  2. On the frontend: bun link vike-solid then bun dev

@brillout
Copy link
Member

Solid usually doesn't like "early returns" [...] will lose out on reactivity

I see. It's beyond my Solid knowledge but maybe Joël has some insights.

@Blankeos
Copy link
Contributor Author

Do you know what code controls the client-side rendering under the hood (Like when the config is set to: ssr: false)? I'm thinking it might be what's causing the hydration errors.

I don't think it's getPageElement since when I make changes on it, it doesn't get reflected on the rendered html on the frontend. Is it still controlled by vike-solid or just by vike?

I noticed there's something inherently weird with vike-solid's client-side rendering at the moment with cumulative layouts, I just can't figure out which part of the code is causing it. I did the same example on vike-react as well, it works as expected. Problem seems to only happen for vike-solid.

ssr-false-vike-solid.mp4

@brillout
Copy link
Member

In case you didn't see it already: https://github.com/vikejs/vike-solid/blob/main/vike-solid/renderer/onRenderClient.tsx.

If you want to experiment starting from a clean slate: https://vike.dev/solid#custom-integration in particular the first two are minimal.

Is it still controlled by vike-solid or just by vike

Since vike is completely agnostic to Solid, I presume the issue to live in vike-solid land.

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 14, 2024

I've finally done it!!!! AHHHH!

I don't know how I fixed it. Apparently it was getPageElement all along.

The blockers I was facing were due to some:

  • weird interoperation between pnpm and bun between my vike-solid and my local project when I pnpm build and bun link. (resolution was just to use pnpm)
  • browser caching or something (not completely sure, but I just set Disable Cache on the browser during development)

Apparently the recursive solution worked (it kept reactivity, the others didn't but feel free to try as well--it may have just been my dev environment acting up again).

vike-solid-nested-finally-works.mp4

Should be ready for review now. Feel free to test it out here:

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 14, 2024

Oh right, forgot. I might also have to add a "nested layouts" tab in the /examples folder similar to what you did on vike-react. (Let me know if you need that)

@brillout
Copy link
Member

Nice! 💯

Very much looking forward to have Nested Layouts also for vike-solid

Since I ain't that familiar with Solid it's probably we wait on Joël's review next week.

@Blankeos Blankeos force-pushed the blankeos/dev branch 2 times, most recently from e43ac4c to 697a0b0 Compare June 16, 2024 10:42
@Blankeos
Copy link
Contributor Author

Just amended a change. Trimmed some unnecessary lines of code from where I referenced it.

Noticed something weird when it's pnpm build and pnpm preview though. Don't know if it's a vike bug (based on the logs). But I tried to reproduce it on react, and it doesn't happen for vike-react.

Wondering what could cause this though since it works perfectly on dev. But not on build.

I made a repro here: https://github.com/blankeos/solid-vike-nested-layouts

  • Go to /dashboard (Nested layout 2-levels deep)
  • Then go (Nested layout 1-level deep)
Error: [vike@0.4.176][Bug] You stumbled upon a Vike bug. Go to https://github.com/vikejs/vike/issues/new and copy-paste this error. A maintainer will fix the bug (usually under 24 hours).
    at de (entry-client-routing.ByC2kXXH.js:2:1671)
    at s (entry-client-routing.ByC2kXXH.js:3:841)
    at entry-client-routing.ByC2kXXH.js:3:19937
    at Array.map (<anonymous>)
    at entry-client-routing.ByC2kXXH.js:3:19898
    at Array.forEach (<anonymous>)
    at Tr (entry-client-routing.ByC2kXXH.js:3:19488)
    at $e (entry-client-routing.ByC2kXXH.js:3:49912)
    at async p (entry-client-routing.ByC2kXXH.js:4:12564)
    at async Q (entry-client-routing.ByC2kXXH.js:4:11817)
image

@brillout
Copy link
Member

Error: [vike@0.4.176][Bug] You stumbled upon a Vike bug.

I cannot reproduce with following steps:

  1. pnpm run dev
  2. Click Dashboard
  3. Click Settings

I don't see any error in the browser dev console nor in the terminal.

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 17, 2024

I cannot reproduce with following steps:

pnpm dev

On dev it works fine. It's when it's built that's a problem.

Have you tried pnpm build pnpm preview? Then

  1. Dashboard
  2. Settings
  3. Home (you should see an unexpected behavior at this point)

@brillout
Copy link
Member

Indeed I can reproduce now. It seems to be a Vike bug, let me dig & fix.

@brillout
Copy link
Member

Actually, it seems to be caused by your changes.

Instead of directly manipulating pageContext.config.Layout, can you manipulating a copy instead?

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 17, 2024

Instead of directly manipulating pageContext.config.Layout, can you manipulating a copy instead?

Interesting. Thanks for investigating! Although, .toReversed() is an immutable operation and doesn't really manipulate the original array. Just returns a new one, so it's a bit weird.

I already create a copy called layoutStore.

I'll see what I can do about it again later. Thanks for replying on Discord btw.

@brillout
Copy link
Member

If you inject a console.log() before the assert() fails (third line in the stack trace), you'll see that there is some Solid proxy going on.

@Blankeos Blankeos force-pushed the blankeos/dev branch 2 times, most recently from 86906ac to b1ffd3f Compare June 17, 2024 14:56
@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 17, 2024

I'm kinda stuck on this guys.

Right now, it works perfectly on pnpm dev but not in pnpm build && pnpm preview. Can't see errors anymore so it's just super vague atm.

Currently developing on the examples/basic project if you maybe want to reproduce.

@brillout
Copy link
Member

Ryan Carniato had a quick look at it: https://x.com/RyanCarniato/status/1802764667754013109.

BREAKING CHANGE: The `Layout` setting cannot be overriden anymore because it's now cumulative, see:
 - https://vike.dev/Layout#multiple-layouts
 - https://vike.dev/Layout#nested-layouts
@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 18, 2024

Great news @brillout , I think I've finally done it! I've tested mostly always on pnpm build and pnpm preview. It's finally working the way it's expected. Do test it if you have time.

Summary of how I got to the resolution:

  • Upgrading the solid-js version from 1.8.16 to 1.8.17 inside the example/basic seemed to fix the issue where the page doesn't render when navigating. (I think this is just a version mismatch during pnpm link so it gets confused that two solid-js versions are installed in the project)

  • It seemed that the reconcile inside onRenderClient was causing the problems. So I removed it and just do setPageContext(pageContext) directly. (Although, I'm not sure if there are any negative implications to this).

  • I think doing reconcile on an [() => {}, () => {}] data structure is what might be causing those problems because it doesn't know how to diff it probably.

  • This time I just reconcile the pageContext.config.Layout specifically inside the <Layout /> component so it can persist the state when it tries to re-evaluate the new layouts.

  • I also reverse the Layouts in onRenderClient and onRenderHTML so it's ready to use in the getPageElement.

@brillout
Copy link
Member

Neat! 💯 Let's see what Joël thinks.

(FYI Solid is quite responsive to bugs, so I wouldn't hesitate reporting bugs.)

@magne4000
Copy link
Member

magne4000 commented Jun 18, 2024

@Blankeos I was testing a few things on my side, and also came to the conclusion that reconcile doesn't work as intended with vike context. I stumbled upon an issue that seems to explain why.

I also managed to kinda fix the issue on my side also by trying to clone pageContext (which is not that easy when you have functions and Proxies). Your solution is simpler, so we'll keep it, but if a similar issue arise, I think that cloning the whole pageContext would be a more robust solution.

@magne4000
Copy link
Member

I removed the usage of toReversed() in order to keep compatibility with node@18. We just loop in reverse order instead.

@magne4000
Copy link
Member

With those last changes, I think we're good to merge! @Blankeos good for you too?

@Blankeos
Copy link
Contributor Author

Blankeos commented Jun 18, 2024

Hi @magne4000! Thanks for the additional changes!

I see, I remember using .toReversed() initially because Solid stores prevent you from mutating the original array. But since we don't reverse from the store anymore, it should be good.

Were there no problems with using .reverse()? If not, I'm good to go as well!

@magne4000
Copy link
Member

magne4000 commented Jun 18, 2024

Were there no problems with using .reverse()?

I don't know I didn't try. But mutating pageContext should be avoided, so I would have used something like [...array].reverse() instead if needed.

@magne4000 magne4000 merged commit 127a3ab into vikejs:main Jun 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants