fix: do not skip app-wrapper for sibling routes

[csvn]

This PR adds a unit test for the issue in #2910. I have not found a solution yet, since I'm not very familiar with the fsRoutes code, and what it is doing. But a thought a unit test with the failing code could help to find a bug fix.

Edit: I managed to fix the failing test by removing a couple of lines, but I'm not too familiar with the code so I'm not sure if it's correct. It seems to pass all the tests still...

Closes #2910

[csvn]

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.

[csvn]

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...

[csvn]

@marvinhagemeister I think this PR is ready for review. I don't see any failing tests, and it looks like the test I added now passes so this should likely fix #2910 too.

[marvinhagemeister]

Great catch! Seems like always setting hasApp = false was a bit heavy handed. Happy to hear that this was all that's needed to fix it 👍

[marvinhagemeister]

Ah wait kinda wondering if there is a case where a route says to skip the app wrapper, but the next route in the list doesn't. In that case we need to ensure that the app wrapper template is still applied.

[csvn]

Ah wait kinda wondering if there is a case where a route says to skip the app wrapper, but the next route in the list doesn't. In that case we need to ensure that the app wrapper template is still applied.

I think that's exactly what I tried to accomplish with my change? To me skipApp is used for that, and hasApp is whether or not an _app file exists at all. I don't think "_app exists" should ever be changed after set to true.

• After my changes, hasApp is initialized to false, and only ever changes once to true if /_app is encountered.
• Nested for loops are running over two route modules, and routeMod seems to be all routes, and mod the current "special" route (e.g. _app, _layout etc)
  • skipApp (let skipApp = !!routeMod.config?.skipAppWrapper;) and mod.config?.skipAppWrapper, are these ones together not enough to determine if the app wrapper should be included?
• I don't really understand what the components.shift() part does 😅

It at least looks off to me if hasApp is set to false, because if routes 1, 2 and 3 are processed in that order, if 1 uses skipAppWrapper: true, then hasApp becomes false and 2 and 3 can never have the app-wrapper.

Not sure I understood your question. But I don't think I've dug into fs_routes/mod.ts enough to understand it more than that. 🤔

[csvn]

Perhaps hasApp could be renamed appRouteExists, and useAppWrapper/useAppComponent instead of skipApp. Maybe that would make it clearer? I think it should likely be enough to only ever change skipApp in current for loop, because no other route should be able to make permanent changes to another route?

[marvinhagemeister]

Ah you're totally right. I confused hasApp with skipApp, I guess a sign that more distinct names would be better 😅

[csvn]

@marvinhagemeister there's been quite a few fixes and changes in the last few weeks. Would it be possible to release a new Alpha version soon? The last release was in January, and it would be nice to test out some of the recent bug-fixes with a new JSR release 🙏 😇

(No pressure though 😁)

[marvinhagemeister]

@csvn yep, released alpha 30 earlier today.