refactor: fs-router to use new_createPages & fix staticPaths

[tylersayshi]

Changes:

• pages are built in slices of 2500 to avoid running out of memory when running Promise.all
• this moves fs-router to use new_createPages

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Updated (UTC)
waku	⬜️ Ignored (Inspect)	Visit Preview	Dec 6, 2024 8:49am

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[tylersayshi]

@dai-shi the e2e test is failing because it seems like the 401 error is no longer caught correctly. Is this meant as a responsibility of NewRouter or ErrorBoundary?

Or do I need to try/catch the createElement call on the page component or something else in new_createPages?

[dai-shi]

it seems like the 401 error is no longer caught correctly.

I'm not sure what broke it. Can you look into it? It's probably my oversight somewhere.

[tylersayshi]

@dai-shi for the error boundary issue here... this screenshot shows in order:

1. a console error for the initial 401 for the route (expected)
2. a log I put in checkStatus where we throw the error from: https://github.com/dai-shi/waku/blob/main/packages/waku/src/minimal/client.ts#L31
3. react console.errors then hits the default error boundary: https://github.com/dai-shi/waku/blob/main/packages/waku/src/router/client.ts#L612-L635

The test fails because the error is being thrown from here: https://github.com/dai-shi/waku/blob/main/packages/waku/src/minimal/client.ts#L208

The Root is a parent of the ErrorBoundary used in the ssr-catch-error test, so it makes sense that this would not be caught.

With all of this, I am not sure how we should adjust:
Should the ErrorBoundary for this test be moved to a custom Root component?
Should we be throwing the error from some other component? Like the Slot for the page component?
Something else?

[dai-shi]

The test fails because the error is being thrown from here

Do you mean fetchRsc throws? It should return a promise without throwing.

[tylersayshi]

The test fails because the error is being thrown from here

Do you mean fetchRsc throws? It should return a promise without throwing.

fetchRsc throws because checkStatus throws inside fetchRSC and the error is not caught inside of fetchRsc

[dai-shi]

With all of this, I am not sure how we should adjust

I think the test should pass without modifying. This feels like a bug in new_defineRouter or NewRouter. 🤔

[tylersayshi]

No, when I say passing path, which is the current behavior, it means the root layout takes /foo as the path if the user visits /foo. (unless I misunderstand the current behavior.)

oh, yea that makes sense to me. I think I just misunderstood something you said on discord then. I follow this though now, thanks.

Are you ok with not passing the path to layouts for now though?

[dai-shi]

Are you ok with not passing the path to layouts for now though?

This is technically a breaking change. Can you check README and all examples and see if we describe/use the current capability?

[dai-shi]

Even if we are okay with the breaking change, we should change it in a different PR in advance. It means the new_createPages lies its types.

[dai-shi]

I think dynamic layout should receive path in props. We could get it from context, but it feels like a last resort.

[tylersayshi]

I can add the path back to Layout props for this PR to avoid the breaking change and we can discuss that separately. That seems easier.

[tylersayshi]

I think dynamic layout should receive path in props. We could get it from context, but it feels like a last resort.

I think static layout works too from testing. Would you double check though?

[dai-shi]

avoid the breaking change and we can discuss that separately. That seems easier.

Yeah.

I think static layout works too from testing. Would you double check though?

No, it shouldn't work. We need to use different slotId for different path. Let me try an example.

[dai-shi]

Try this:

diff --git a/examples/21_create-pages/src/components/HomeLayout.tsx b/examples/21_create-pages/src/components/HomeLayout.tsx
index 55e77435..d63ea21a 100644
--- a/examples/21_create-pages/src/components/HomeLayout.tsx
+++ b/examples/21_create-pages/src/components/HomeLayout.tsx
@@ -16,8 +16,9 @@ const Pending = ({ isPending }: { isPending: boolean }) => (
   </span>
 );
 
-const HomeLayout = ({ children }: { children: ReactNode }) => (
+const HomeLayout = ({ path, children }: { path: string, children: ReactNode }) => (
   <div>
+    <h1>Path: {path}</h1>
     <ul>
       <li>
         <Link

Aaaand, I found there's a bug in the current createPages. So, it's not working as expected anyway. Let's fix it in new_createPages.

[dai-shi]

Let's fix it in new_createPages.

Or, change the spec as it's not working correctly anyways.

[tylersayshi]

Let's fix it in new_createPages.

Or, change the spec as it's not working correctly anyways.

what is the bug?

also I see what you mean with it not working for static layouts.

[tylersayshi]

converted to draft while we discuss what to do with Layout path prop

[dai-shi]

LGTM. Huge thanks to your effort.