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

Non-root layouts not loaded in v0.21.8 #1055

Closed
rmarscher opened this issue Dec 10, 2024 · 9 comments · Fixed by #1077
Closed

Non-root layouts not loaded in v0.21.8 #1055

rmarscher opened this issue Dec 10, 2024 · 9 comments · Fixed by #1077
Assignees
Labels
bug Something isn't working

Comments

@rmarscher
Copy link
Contributor

rmarscher commented Dec 10, 2024

I think there's a bug in v0.21.8. The list of found path segments with _layout.tsx files is incorrect. It includes undefined and [object] strings. I think the fix is to use segment.name instead of segment [edit: and use index instead of index - 1].

--- a/dist/router/create-pages.js
+++ b/dist/router/create-pages.js
@@ -185,10 +185,10 @@ export const createPages = (fn)=>{
     };
     const getLayouts = (spec)=>{
         const pathSegments = spec.reduce((acc, segment, index)=>{
-            if (acc[index - 1] === '/') {
-                acc.push('/' + segment);
+            if (!acc[index] || acc[index] === '/') {
+                acc.push('/' + segment.name);
             } else {
-                acc.push(acc[index - 1] + '/' + segment);
+                acc.push(acc[index] + '/' + segment.name);
             }
             return acc;
         }, [

There should probably be a test case to catch this...

@dai-shi
Copy link
Owner

dai-shi commented Dec 10, 2024

@tylersayshi

@rmarscher
Copy link
Contributor Author

I think this patch is missing something. I'm still getting errors. Working on debugging...

@rmarscher
Copy link
Contributor Author

index - 1 needed to be index. I edited the diff in my original post above.

@tylersayshi
Copy link
Contributor

Ah, yea, I don't think we have any e2e tests to check nested layouts.

Sorry about this, I can help look into a fix today

@dai-shi dai-shi added the bug Something isn't working label Dec 13, 2024
dai-shi pushed a commit that referenced this issue Dec 13, 2024
@dai-shi
Copy link
Owner

dai-shi commented Dec 13, 2024

#1063

@rmarscher
Copy link
Contributor Author

Sorry... @tylersayshi I think we missed something here. I had to apply this patch to v0.21.9.

diff --git a/dist/router/create-pages.js b/dist/router/create-pages.js
index f4e7afb30f4f1a6c7cf6172692c9dcc95e22e210..bcbf749487dbfd24a190f3157806905f6019af9b 100644
--- a/dist/router/create-pages.js
+++ b/dist/router/create-pages.js
@@ -188,7 +188,7 @@ export const createPages = (fn)=>{
             if (index === 0) {
                 acc.push('/' + segment.name);
             } else {
-                acc.push(acc[index - 1] + '/' + segment.name);
+                acc.push(acc[index] + '/' + segment.name);
             }
             return acc;
         }, [

@dai-shi
Copy link
Owner

dai-shi commented Dec 17, 2024

I wonder if we can write a test for it.

@dai-shi dai-shi reopened this Dec 17, 2024
@rmarscher
Copy link
Contributor Author

I wonder if we can write a test for it.

Yeah, I assumed https://github.com/dai-shi/waku/blob/main/e2e/21_create-pages_standalone.spec.ts#L36-L39 would catch...

For my error, my layout was two levels deep - so maybe we can reproduce by adding /nested/baz/layout.tsx with a /nested/baz/[id]/view.tsx page route.

@tylersayshi
Copy link
Contributor

tylersayshi commented Dec 18, 2024

 -                acc.push(acc[index - 1] + '/' + segment.name);
 +                acc.push(acc[index] + '/' + segment.name);

This part of the diff I intentionally omitted, it's not correct, but thanks for sharing still because this does help explain the issue.

I'll add a test to replicate the further nested path and make sure layouts are applied there as well. 👀

Correction: You were right and this is my own off by one error 🤦. I'll send up a PR

tylersayshi added a commit to tylersayshi/waku that referenced this issue Dec 18, 2024
Adds tests for layouts with nesting and slugs

fixes dai-shi#1055
dai-shi pushed a commit that referenced this issue Dec 18, 2024
Adds tests for layouts with nesting and slugs

fixes #1055

---------

Co-authored-by: Tyler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants