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

fix #144: Nested layouts infinite recursive call. #145

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

Blankeos
Copy link
Collaborator

@Blankeos Blankeos commented Dec 19, 2024

Fixes #144

TL;DR; These are the two issues fixed:

  • Infinite Recursion (Error: Maximum call stack size exceeded)
  • Layout states not persisting when navigating into new [+Layout.tsx, +Layout.tsx, +Layout.tsx]

@Blankeos Blankeos marked this pull request as draft December 19, 2024 13:01
@Blankeos Blankeos force-pushed the fix/infinite-recursion-nested-layouts branch from 2563292 to 269d60c Compare December 19, 2024 15:10
@Blankeos Blankeos marked this pull request as ready for review December 19, 2024 15:10
@Blankeos
Copy link
Collaborator Author

Have fully tested this. It works as expected.

I wrote most of my notes as I solved the bugs in the issue comments.

Feel free to check there.

@Blankeos
Copy link
Collaborator Author

Blankeos commented Dec 19, 2024

Other solutions I attempted to fix this.

This was actually the solution that made me arrive to the simpler solution in this PR. This is longer because it rewrites the the recursion (I thought it was the culprit).

  • I initially thought it was the createComponent recursion that was causing the bug for non-persistence of state.
  • Got this solution from Claude. This is interesting because this showcases how you can essentially do "early returns" in SolidJS (which is supposedly impossible) . Using the <Switch> and <Match> primitives.
  • In the end, I noticed it was because this is reversing and accessing the array using [i] instead of .at(i). Which actually solved the problem in the current implementation.
function RecursiveWrapper(props: { wrappers: FlowComponent[], children: JSX.Element, index: number }) {
 return (
   <Switch fallback={props.children}>
      <Match when={props.index < props.wrappers.length}>
        {(() => {
          // const CurrentComponent = props.wrappers.at(-(props.index + 1)) ?? Passthrough; // Access in reverse (has unexpected behaviors)
          const CurrentComponent = props.wrappers[props.index] ?? Passthrough;
          
          return (
            <CurrentComponent>
              <RecursiveWrapper wrappers={props.wrappers} index={props.index + 1}>
                {props.children}
              </RecursiveWrapper>
            </CurrentComponent>
          );
        })()}
      </Match>
    </Switch>
 )
}


 function Wrapper(props: { children: JSX.Element }) {
   const pageContext = usePageContext();
 
   const [wrappers, setWrappers] = createStore<FlowComponent[]>([]);
 
   createComputed(() => {
     setWrappers(
       reconcile([
         // Inner wrapping
         ...(pageContext.config.Layout || []),
         // Outer wrapping
         ...(pageContext.config.Wrapper || []),
      ]),
        ].toReversed()),
     );
   });
  
  return (
    <RecursiveWrapper wrappers={wrappers} index={0}>    
      {props.children}
   </RecursiveWrapper>
  )
}

@brillout
Copy link
Member

@magne4000 WDYT?

@magne4000
Copy link
Member

This looks perfect, thanks!

Note: I don't know if those are limitations of SolidJS or actual bugs, we should probably look into that.

@magne4000 magne4000 merged commit 985f346 into vikejs:main Dec 19, 2024
4 checks passed
@Blankeos
Copy link
Collaborator Author

Blankeos commented Dec 19, 2024

Thanks Joël! I agree, planning to make small repros of the issue those limitations with SolidJS to confirm as well.

Also btw I think I forgot to pnpm format so the CI failed.

@magne4000
Copy link
Member

Published as 0.7.8

@magne4000
Copy link
Member

Also btw I think I forgot to pnpm format so the CI failed.

No worries, I fixed it

@brillout
Copy link
Member

@Blankeos I talked with Joël and, if that's something you'd be up for, we would like to add you as one of the maintainers of vike-solid. Let me know if that sounds appealing to you and I'll add you to the repository and the npm package.

@Blankeos
Copy link
Collaborator Author

Hi @brillout! would love to. Feel free to contact me on Discord :)

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.

Upgrading from 0.7.6 to 0.7.7 breaks Nested/Cumulative Layouts.
3 participants