Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
dai-shi
left a comment
There was a problem hiding this comment.
I haven't looked the MyRouter code, but it feels like a great start.
I assume it's still work in progress, and convert to draft for now.
|
|
| useEffect(() => { | ||
| const callback = (path: string, query: string) => { | ||
| const fn = () => { | ||
| const url = new URL(window.location.href); | ||
| url.pathname = path; | ||
| url.search = query; | ||
| url.hash = ''; | ||
| changeRoute(parseRoute(url), { | ||
| skipRefetch: true, | ||
| shouldScroll: false, | ||
| }) | ||
| .catch((err) => { | ||
| console.log('Error while handling location listeners:', err); | ||
| }) | ||
| .finally(() => { | ||
| if (path !== '/404') { | ||
| window.history.pushState( | ||
| { | ||
| ...window.history.state, | ||
| waku_new_path: url.pathname !== window.location.pathname, | ||
| }, | ||
| '', | ||
| url, | ||
| ); | ||
| } | ||
| }); | ||
| }; | ||
| if (refetching.current) { | ||
| refetching.current.push(fn); | ||
| } else { | ||
| startTransition(fn); | ||
| } | ||
| }; | ||
| locationListeners.add(callback); | ||
| return () => { | ||
| locationListeners.delete(callback); | ||
| }; | ||
| }, [changeRoute, locationListeners]); |
There was a problem hiding this comment.
I didn't understand this locationListeners. From commit history this is a workaround, but how?
(sorry for too many notification, github ui sucks)
There was a problem hiding this comment.
AFAIR, this was introduced so that we changeRoute synchronously with other React rendering. We should probably create an e2e test case that covers it.
There was a problem hiding this comment.
create an e2e test case that covers it.
Any existing example?
btw I'll left the history here. I don't know how to convert this.
There was a problem hiding this comment.
create an e2e test case that covers it.
Any existing example?
Actually, there seems some e2e tests fail if we comment out locationListeners.add(callback);.
It's now a working demo, and maybe you have better intuition about cleaning up, or if lacking what is tolerable. So look forward to you reviewing opinion! |
|
I want to make startTransition integrated and compulsory, but API change is out of scoop of the pr. It should be ready now. I somehow managed to test it on Chrome. |
dai-shi
left a comment
There was a problem hiding this comment.
If the goal of this PR is to keep the current Router API surface, it looks like a minimal change, and should be good even though I didn't have closer look around navigation api.
The copy-pasted code isn't really comfortable, so I'll create a follow-up PR to expose some internal utils/types with unstable prefix. (In the long term, I guess this might not be kept.)
Our current Router with history api still has some issues, and we'll fix them, but I'll keep in mind for the navigation api.
Yes.
You said "I don't want to mix", do you have a re-consideration? unstable_api is favourable than a copy-paste version for me. Still, a working playground is valuable as a base for further API change, etc. |
|
Yes. I don't want to mix. I'll see what I'm comfortable with. This PR should go as-is. |
commit: |
b059ccb to
e8349aa
Compare
|
Playwright WebKit version is 26.0. It supports Navigation API since 26.2. Copy-paste creates some type error, which is hard to fix. (Maybe False positive is annoying. |
Add
Sounds like we should wait for Playwright update. |
|
TODO:
|
Well, I made another branch with more thorough clean up. It will fix them all: scroll, time of changing url, race condition. I deleted everything I don't understand. I'll see if this can pass the e2e test. https://github.com/Master-Hash/waku/tree/rash-router Update: |
|
My branch has passed all e2e test! https://github.com/Master-Hash/waku/tree/rash-router https://github.com/Master-Hash/waku/actions/runs/20690707953/job/59398409173 I'll write up the details as soon as possible. If you like it, I'll create a draft pull request. UPDATE: write-up |
|
Not sure if referenced, but async-react's client router has concise comparison of history and navigation API https://github.com/rickhanlonii/async-react/blob/c971b2fa57785dc2014251ec90d3c18cb7a958c6/src/router/index.jsx For example, |
Thank you for mention it. I believe the solution I coined is exactly the same with async-react. |
I didn't notice this was added. And, I misunderstood that you were solving with History API for this 👇 .
For Navigation API solution, feel free to update this PR. Or, maybe we can create a separate package. Which do you prefer? In any case, I think we need to solve our current router with History API in the meantime. |
I'm using a pnpm patch on my own website. https://github.com/Master-Hash/hashland/blob/vite/patches/waku.patch I can keep my fork, so an official separate package isn't necessary for me. (You can also see how Navigation API works in production on that website)
Yes, but I hate history API:( |
That's unfortunate. I don't think we can throw it away yet. Let's hope we find another contributor. I would like to migrate to Navigation API eventually, but when I were to do it, I might reconsider some requirements. I want our client router code to be thinner. Not sure. Will see. |
I've cut it ~200 lines of code down. Now the navigation itself is optimal, network is also, only error handler (404, redirection) needs a full reconsideration. |
|
Would you please update this PR with your latest approach? Not changing the current router, but as the 46_navigation_api example. |
I'll try something. This will help me understand the problem space as well as #1881. |
39f5bea to
a234600
Compare
Or, a new subpath? I'm in favor of this. |
|
It doesn't happen anytime soon. By the way, when do you think we can drop history api completely? I think if we were to support navigation api, we might need to have history api fallback for quite a while. We'll probably learn from other frameworks. Until then, I would like to finish implementing the history api based router (which can then turn into a separate package in the future.) |
|
TODO:
|
~2 years later? |
|
I'll try my best to catch up with upstream client.tsx. Some part of client.tsx is no longer needed in Navigation API, like I also have some ideas about It's a little tricky to use separated lib. :( |
|
I believe I'll continue to work on waku-navigation later, but my current focus is fixing the history api-based router. |


Current it lacks:
<Link>component basis1Footnotes
It is automatically by default; set
scroll: "manual"and handle every scroll is the only way to adjust scroll behavior on <Link> component basis, but now it's buggy; currently Firefox's behavior is inconsistent and buggy ↩