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

refactor: fs-router to use new_createPages #1003

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tylersayshi
Copy link
Contributor

I am confident with the current state of new_createPages, but I'd appreciate help testing this further.

This should come after #1000

This includes the commits from #1000 as well since it needs the fixes included over there. The latest commit on this branch is all that's relevant for this PR though.

Copy link

vercel bot commented Nov 15, 2024

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

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Nov 25, 2024 7:48am

Copy link

codesandbox-ci bot commented Nov 15, 2024

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
Copy link
Contributor Author

@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
Copy link
Owner

dai-shi commented Nov 18, 2024

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 tylersayshi marked this pull request as ready for review November 24, 2024 04:38
@tylersayshi tylersayshi marked this pull request as draft November 24, 2024 04:49
@tylersayshi
Copy link
Contributor Author

tylersayshi commented Nov 24, 2024

@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
image

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
Copy link
Owner

dai-shi commented Nov 24, 2024

The test fails because the error is being thrown from here

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

@tylersayshi
Copy link
Contributor Author

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
Copy link
Owner

dai-shi commented Nov 24, 2024

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

dai-shi added a commit that referenced this pull request Nov 25, 2024
…#1013)

add fallback mechanism for #1003.

---------

Co-authored-by: Tyler <[email protected]>
Co-authored-by: Tyler <[email protected]>
@tylersayshi tylersayshi marked this pull request as ready for review November 25, 2024 03:37
@tylersayshi tylersayshi marked this pull request as draft November 25, 2024 06:42
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.

2 participants