Skip to content

fix: do not skip app-wrapper for sibling routes #2911

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.

Sign up for GitHub

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

Merged
marvinhagemeister merged 2 commits into from
May 13, 2025
Merged

fix: do not skip app-wrapper for sibling routes #2911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4de8d4a
test: add app-wrapper test for sibling config routes
csvn May 10, 2025
c484500
fix: do not skip app wrapper permanently
csvn May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/plugins/fs_routes/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,12 +221,10 @@ export async function fsRoutes<State>(

// _app template
if (skipApp && mod.path === "/_app") {
hasApp = false;
continue;
} else if (!skipApp && mod.config?.skipAppWrapper) {
skipApp = true;
if (hasApp) {
hasApp = false;
// _app component is always first
components.shift();
Comment on lines 223 to 229

@csvn csvn May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's correct just to remove these lines. It looks wrong to me to set hasApp to false if we're looping over every route, since a latter route may have the app wrapper disabled always.

This change makes the test I added pass anyway...

}
Expand Down
50 changes: 50 additions & 0 deletions src/plugins/fs_routes/mod_test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,56 @@ Deno.test("fsRoutes - route overrides _app", async () => {
expect(doc.body.firstChild?.textContent).toEqual("route");
});

Deno.test("fsRoutes - route overrides _app does not affect other routes", async () => {
const server = await createServer({
"routes/_500.tsx": {
config: { skipInheritedLayouts: true },
default: () => <>error!</>,
},
"routes/_app.tsx": {
default: (ctx) => (
<div>
app/<ctx.Component />
</div>
),
},
"routes/_layout.tsx": {
default: (ctx) => (
<>
layout/<ctx.Component />
</>
),
},
// By having "bar" come before "foo", this results in a bug
// where the app wrapper is missing from "foo".
"routes/bar.tsx": {
config: {
skipAppWrapper: true,
skipInheritedLayouts: true,
},
default: () => <>bar</>,
},
Comment on lines +581 to +589

@csvn csvn May 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If route/bar.tsx came after routes/foo.tsx in the record, the issue disappears. Since iteration over the files happens in key-order (for test method createServer) it seems to be some connection to the order FS will walk over the directory for this issue to occur in a real app.

"routes/index.tsx": { default: () => <>index</> },
"routes/foo.tsx": { default: () => <>foo</> },
});

const [indexRes, fooRes, barRes] = await Promise.all([
server.get("/"),
server.get("/foo"),
server.get("/bar"),
]);
const [index, foo, bar] = await Promise.all([
indexRes.text(),
fooRes.text(),
barRes.text(),
]);
expect(parseHtml(index).body.firstChild?.textContent).toEqual(
"app/layout/index",
);
expect(parseHtml(foo).body.firstChild?.textContent).toEqual("app/layout/foo");
expect(parseHtml(bar).body.firstChild?.textContent).toEqual("bar");
});

Deno.test("fsRoutes - handler return data", async () => {
const server = await createServer({
"routes/index.tsx": {
Expand Down
Loading