fix: update url after route change #1459

rmarscher · 2025-06-05T20:55:28Z

vercel · 2025-06-05T20:55:32Z

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

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Jul 9, 2025 1:14pm

codesandbox-ci · 2025-06-05T20:56:04Z

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.

Part of the fix for #1437. It turns out that #1437 is reporting multiple issues: 1. Showing preview element content instead of a transition when navigating to a previously visited page. That issue is replicated with the e2e tests in this PR and fixed. 2. Updating the url before changing the route. I'm expecting the url to not update until the new page has loaded. I opened #1459. 3. The changeRoute function completes before the page component has streamed any data. The third issue is hard to test without using unstable_events. --------- Co-authored-by: daishi <[email protected]>

rmarscher · 2025-06-06T05:32:51Z

Why does 9791f07 cause e2e tests like "broken-links: static server > redirect" to fail? Is there some behavior that depends on the history update?

I commented out the new e2e tests for scroll position and url updating so we can focus on that issue. Then maybe we can figure out how to fix the refetch promise in changeRoute and re-enable these e2e tests.

dai-shi · 2025-06-20T02:55:33Z

@rmarscher Please merge main.

…route-change

rmarscher · 2025-06-21T07:45:09Z

I think I got a little closer by using finally to make sure it always reaches the code that updates the url. However with a client-side redirect, it seems to switch to the final url and then back to the originally requested url.

So with the broken-links example, running it with a dev server, click on "Correct redirect" and the url history will go to /exists and then /redirect. It would be nice to know that a redirect changed it somehow and skip updating.

I added RSC redirects in 4265f05 to be able to test it with a dev server. The RSC redirect was only in the public/serve.json.

dai-shi · 2025-06-23T01:53:50Z

If things around redirect handling seem too hard to implement or too complicated to maintain, I think our idea should be to reduce the requirement. For example, my understanding around client redirect was not enough when we added the support for redirect config in serve.json, which is technically outside the framework and which depends on rsc path convention. Maybe, it's time to revisit. (and, should do before v1-alpha.)

dai-shi · 2025-07-01T23:41:53Z

@rmarscher What's the current status?

rmarscher · 2025-07-04T05:36:42Z

@dai-shi I think all of the tests are passing except one. Router events seem to be firing twice on the first navigation after loading a dynamic page. They do not do that on subsequent page loads or if the static page is loaded first.

cc/ @tylersayshi

e2e/use-router.spec.ts:119:5 › useRouter › calls route change event handlers › on dynamic pages 

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  - 0
    + Received  + 2

      Array [
        "Route change started",
    +   "Route change started",
    +   "Route change completed",
        "Route change completed",
      ]

rmarscher · 2025-07-06T14:49:22Z

changeRoute is called twice on first navigation to /static after loading /dynamic in the use-router fixture.

I found that this is because of the "locationListeners". After the RSC fetch returns, it is looking at the routeData and then running locationListeners, if necessary. I think this is to support server-side redirects or just an extra check to make sure that the window.location and current page elements are in sync?

Before this PR, the location was updated before this code was hit so the check to see if the location update passed. The one labeled - "FIXME this check here seems ad-hoc" ;-)

Now it's failing so we'll need another way to determine if we are already in the middle of a route change.

rmarscher · 2025-07-06T15:19:10Z

It's looking good now @dai-shi @tylersayshi. Maybe just a flaky windows test failure.

dai-shi

@rmarscher Thanks for working on this! It looks pretty clean. Just left a few comments.

@tylersayshi Would you like to give a review with another eye?

packages/waku/src/router/client.ts

tylersayshi

rest lgtm 👍

tylersayshi · 2025-07-08T23:13:55Z

e2e/fixtures/broken-links/src/redirects.ts

+  const redirects = {
+    '/redirect': '/exists',
+    '/RSC/R/redirect.txt': '/RSC/R/exists.txt',
+    '/broken-redirect': '/broken',
+    '/RSC/R/broken-redirect.txt': '/RSC/R/broken.txt',
+  };


should this be declared outside the function scope? or does it not really matter?

🤷 Yeah, probably. technically the /RSC/R prefix and .txt extensions should probably be imported from waku constants or use a function that converts a normal path to an RSC path.

@dai-shi what do you think about exposing encodeRscPath() for this?

tylersayshi · 2025-07-08T23:15:55Z

packages/waku/src/router/client.ts

@@ -482,6 +491,18 @@ const Redirect = ({ to, reset }: { to: string; reset: () => void }) => {
      })


Just curious: Is the setTimeout hack still needed with the new route change behavior?

I just tried and there are e2e tests that break if I remove it.

tylersayshi · 2025-07-08T23:16:47Z

packages/waku/src/router/client.ts

+        try {
+          await changeRoute(parseRoute(url), {
+            skipRefetch: true,
+            shouldScroll: false,
+          });
+        } finally {
+          if (path !== '/404') {
+            window.history.pushState(
+              {
+                ...window.history.state,
+                waku_new_path: url.pathname !== window.location.pathname,
+              },
+              '',
+              url,
+            );
+          }
        }


why not use .finally here like the other use?

Good question. I should have not switched it to an async def. I'll fix.

Oh... it's so the error can fall through. I think we need it.

dai-shi

@rmarscher Can you clarify again when we need .finally {} and when not?

dai-shi · 2025-07-08T23:57:42Z

packages/waku/src/router/client.ts

+            unstable_startTransition: startTransitionFn,
+          });
+        } catch (err) {
+          console.error('Error while navigating to new route:', err);


We have console.error here too.

rmarscher · 2025-07-09T01:23:34Z

@rmarscher Can you clarify again when we need .finally {} and when not?

Now that window.history.pushState is called after awaiting changeRoute(), we need to use finally to make sure we update the url even if changeRoute throws an exception.

It is not necessarily needed for the useRouter() APIs because the developer can catch the error from changeRoute and handle it on their own.

The try { await changeRoute(); } finally { pushState } pattern is used for useTransition functions (<Link> ).

The changeRoute().catch(() => { console.error }).finally(() => { pushState }) pattern is used for useEffect and startTransition functions (<Redirect>, locationListeners ).

changeRoute is called without pushState in the popstate window event listener - because it's reacting to the url already being updated. Similarly, reload() and <NotFound> do not call pushState.

I think that covers all of the uses of changeRoute in waku/router/client.

pkg-pr-new · 2025-07-09T01:32:10Z

Open in StackBlitz

npm i https://pkg.pr.new/wakujs/waku@1459

commit: 68b4db3

rmarscher · 2025-07-09T01:41:54Z

Hmm... the locationListeners use https://react.dev/reference/react/startTransition - they probably should use the .finally() syntax instead of async. I'm going to change that.

rmarscher · 2025-07-09T01:54:24Z

OK. I think I have addressed all of the comments. Thanks for the reviews.

dai-shi

Thanks for working on it. It's a little bit unfortunate to see things become complicated, but can't be helped. Any ideas to fix/mitigate later are welcome.

rmarscher · 2025-07-09T13:55:17Z

Thanks for working on it. It's a little bit unfortunate to see things become complicated, but can't be helped. Any ideas to fix/mitigate later are welcome.

Maybe updating history should move into changeRoute.

changeRoute(url, { history: 'push' });

The history option could be 'push', 'replace' or false.

I'm not sure about unstable_startTransition in changeRoute options. I'm not sure a transition function should happen inside changeRoute. The place that calls changeRoute should wrap it in a transition - which we are doing in Link onClick. And when devs use the useRoute() APIs, they can wrap those in a useTransition hook if they want.

dai-shi · 2025-07-09T14:41:06Z

I'm not sure about unstable_startTransition in changeRoute options.

Maybe we don't need it in the future. One motivation was to support view transition api, but it will be supported by React natively.

vercel bot deployed to Preview June 5, 2025 20:56 View deployment

This was referenced Jun 5, 2025

Routing completes before new page component has sent anything #1437

Open

fix: continue transition with waku/router Link #1458

Merged

dai-shi mentioned this pull request Jun 6, 2025

v1 Roadmap #24

Open

fix: update url after awaiting a route change

9791f07

rmarscher force-pushed the fix/update-url-after-route-change branch from 4102547 to 9791f07 Compare June 6, 2025 04:42

vercel bot deployed to Preview June 6, 2025 04:43 View deployment

chore: long suspense e2e scroll position

f096bc0

rmarscher mentioned this pull request Jun 6, 2025

fix: make scroll reset happen only after navigation #1399

Closed

rmarscher added 2 commits June 6, 2025 01:11

chore: long suspense e2e url update

1275af9

Revert url and scroll long suspense e2e tests for now

5fba52c

Merge remote-tracking branch 'origin/main' into fix/update-url-after-…

5170b44

…route-change

vercel bot deployed to Preview June 21, 2025 05:42 View deployment

rmarscher added 2 commits June 21, 2025 02:19

wip: finally block to update url

17c630f

broken-links redirect middleware for rsc

4265f05

Merge branch 'main' into fix/update-url-after-route-change

96c67c6

vercel bot deployed to Preview July 3, 2025 19:01 View deployment

rmarscher added 2 commits July 3, 2025 15:20

Fix broken-link fixture redirect map

8162e49

Skip history update if already updated during routing

9f59cef

vercel bot deployed to Preview July 4, 2025 05:08 View deployment

rmarscher changed the title ~~wip: update url after route change~~ fix: update url after route change Jul 4, 2025

add requestedRouteRef for locationListeners

b8c84f2

vercel bot deployed to Preview July 6, 2025 15:02 View deployment

dai-shi reviewed Jul 8, 2025

View reviewed changes

packages/waku/src/router/client.ts Outdated Show resolved Hide resolved

packages/waku/src/router/client.ts Outdated Show resolved Hide resolved

Don't update to new path if error from router.push

d53a7b0

vercel bot deployed to Preview July 8, 2025 17:36 View deployment

tylersayshi reviewed Jul 8, 2025

View reviewed changes

dai-shi reviewed Jul 8, 2025

View reviewed changes

Remove unnecessary console error

55deb2e

vercel bot deployed to Preview July 9, 2025 01:28 View deployment

Attempt remove async for location listeners

e320e96

vercel bot deployed to Preview July 9, 2025 01:43 View deployment

Move redirects constant to outer scope

42e078b

vercel bot deployed to Preview July 9, 2025 01:50 View deployment

Merge branch 'main' into fix/update-url-after-route-change

d517463

vercel bot deployed to Preview July 9, 2025 01:57 View deployment

Merge branch 'main' into fix/update-url-after-route-change

68b4db3

vercel bot deployed to Preview July 9, 2025 13:14 View deployment

dai-shi approved these changes Jul 9, 2025

View reviewed changes

dai-shi linked an issue Jul 9, 2025 that may be closed by this pull request

Routing completes before new page component has sent anything #1437

Open

dai-shi merged commit 43473c6 into wakujs:main Jul 9, 2025
48 of 49 checks passed

rmarscher deleted the fix/update-url-after-route-change branch July 9, 2025 15:03

rmarscher mentioned this pull request Jul 9, 2025

chore: refactor router client history update #1515

Draft

		@@ -482,6 +491,18 @@ const Redirect = ({ to, reset }: { to: string; reset: () => void }) => {
		})

Uh oh!

fix: update url after route change #1459

fix: update url after route change #1459

Uh oh!

Conversation

rmarscher commented Jun 5, 2025

Uh oh!

vercel bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmarscher commented Jun 6, 2025

Uh oh!

dai-shi commented Jun 20, 2025

Uh oh!

rmarscher commented Jun 21, 2025

Uh oh!

dai-shi commented Jun 23, 2025

Uh oh!

dai-shi commented Jul 1, 2025

Uh oh!

rmarscher commented Jul 4, 2025

Uh oh!

rmarscher commented Jul 6, 2025

Uh oh!

rmarscher commented Jul 6, 2025

Uh oh!

dai-shi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tylersayshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dai-shi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarscher commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmarscher commented Jul 9, 2025

Uh oh!

rmarscher commented Jul 9, 2025

Uh oh!

dai-shi left a comment

Choose a reason for hiding this comment

Uh oh!

rmarscher commented Jul 9, 2025

Uh oh!

dai-shi commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

vercel bot commented Jun 5, 2025 •

edited

Loading

codesandbox-ci bot commented Jun 5, 2025 •

edited

Loading

rmarscher commented Jul 9, 2025 •

edited

Loading

pkg-pr-new bot commented Jul 9, 2025 •

edited

Loading