-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
new defineRouter #949
new defineRouter #949
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think the new defineRouter implementation reads really well!
This really helps to see what you meant by simplifying the structures in the previous version.
export function NewServerRouter({ route }: { route: RouteProps }) { | ||
const routeElement = createElement(Slot, { id: getRouteSlotId(route.path) }); | ||
return createElement( | ||
Fragment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this Fragment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, it's to match the component tree with NewRouter. If removing it works, we can remove it. TBH, I don't like NewServerRouter hack. I wish just NewRouter worked.
return fetchRsc(encodeRoutePath('/404')); | ||
} | ||
const data = createData(responsePromise); | ||
Promise.resolve(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this resolve the responsePromise twice? First on L772 then again here? Or is my understanding off with that?
then if it does resolve twice, shouldn't createData just use response
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createData requires a promise, so L772 isn't related with L780.
However, yes, data.then((date) =>
here might work. The reason I added Promise.resolve()
is because data
returned by createData
isn't a real promise. TS types aren't correct here. So, it's just a safe bet. Maybe it's not necessary.
@@ -0,0 +1,156 @@ | |||
import { new_defineRouter } from 'waku/router/server'; | |||
import { Slot, Children } from 'waku/client'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is waku/client
the best place for these? client to me would mean these are targeted towards the scope of client components, but they're just used here for the router to be able to target where to replace with each level of rendering components (regardless of client vs server)
that is if I understand correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are client components. We can embed client components in a server component. It's confusing, but it's how it works. createPages API will hide this complexity anyway.
We would like to redesign
defineRouter
. This is why we haveunstable_
prefix.#885 (comment)
That's the exactly one of the reasons. It shouldn't be 1:1. It can be anything, and it can cache each. This will change waku/router/client too including "shouldSkipList".
Starting with a rough idea.