Skip to content

fix: update url after route change #1459

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Jul 9, 2025
Merged

fix: update url after route change #1459

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9791f07
fix: update url after awaiting a route change
rmarscher Jun 5, 2025
f096bc0
chore: long suspense e2e scroll position
rmarscher Jun 6, 2025
1275af9
chore: long suspense e2e url update
rmarscher Jun 6, 2025
5fba52c
Revert url and scroll long suspense e2e tests for now
rmarscher Jun 6, 2025
5170b44
Merge remote-tracking branch 'origin/main' into fix/update-url-after-…
rmarscher Jun 21, 2025
17c630f
wip: finally block to update url
rmarscher Jun 21, 2025
4265f05
broken-links redirect middleware for rsc
rmarscher Jun 21, 2025
96c67c6
Merge branch 'main' into fix/update-url-after-route-change
rmarscher Jul 3, 2025
8162e49
Fix broken-link fixture redirect map
rmarscher Jul 3, 2025
9f59cef
Skip history update if already updated during routing
rmarscher Jul 4, 2025
b8c84f2
add requestedRouteRef for locationListeners
rmarscher Jul 6, 2025
d53a7b0
Don't update to new path if error from router.push
rmarscher Jul 8, 2025
55deb2e
Remove unnecessary console error
rmarscher Jul 9, 2025
e320e96
Attempt remove async for location listeners
rmarscher Jul 9, 2025
42e078b
Move redirects constant to outer scope
rmarscher Jul 9, 2025
d517463
Merge branch 'main' into fix/update-url-after-route-change
rmarscher Jul 9, 2025
68b4db3
Merge branch 'main' into fix/update-url-after-route-change
dai-shi Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions e2e/fixtures/broken-links/src/redirects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
import type { Middleware } from 'waku/config';

const redirectsMiddleware: Middleware = () => async (ctx, next) => {
switch (ctx.req.url.pathname) {
case '/redirect':
ctx.res.status = 302;
ctx.res.headers = {
Location: '/exists',
};
break;
case '/broken-redirect':
ctx.res.status = 302;
ctx.res.headers = {
Location: '/broken',
};
break;
default:
return await next();
const redirects = {
'/redirect': '/exists',
'/RSC/R/redirect.txt': '/RSC/R/exists.txt',
'/broken-redirect': '/broken',
'/RSC/R/broken-redirect.txt': '/RSC/R/broken.txt',
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (ctx.req.url.pathname in redirects) {
const pathname = ctx.req.url.pathname as keyof typeof redirects;
const url = new URL(ctx.req.url);
url.pathname = redirects[pathname];
ctx.res.status = 302;
ctx.res.headers = {
Location: url.toString(),
};
return;
}
return await next();
};

export default redirectsMiddleware;
140 changes: 84 additions & 56 deletions packages/waku/src/router/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,18 +157,27 @@ export function useRouter() {
},
) => {
const url = new URL(to, window.location.href);
const newPath = url.pathname !== window.location.pathname;
window.history.pushState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
await changeRoute(parseRoute(url), {
shouldScroll: options?.scroll ?? newPath,
});
const currentPath = window.location.pathname;
const newPath = url.pathname !== currentPath;
try {
await changeRoute(parseRoute(url), {
shouldScroll: options?.scroll ?? newPath,
});
} catch (err) {
console.error('Error while navigating to new route:', err);
throw err;
} finally {
if (window.location.pathname === currentPath) {
window.history.pushState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
}
}
},
[changeRoute],
);
Expand All @@ -186,11 +195,14 @@ export function useRouter() {
},
) => {
const url = new URL(to, window.location.href);
const newPath = url.pathname !== window.location.pathname;
window.history.replaceState(window.history.state, '', url);
const currentPath = window.location.pathname;
const newPath = url.pathname !== currentPath;
await changeRoute(parseRoute(url), {
shouldScroll: options?.scroll ?? newPath,
});
if (window.location.pathname === currentPath) {
window.history.replaceState(window.history.state, '', url);
}
},
[changeRoute],
);
Expand Down Expand Up @@ -337,19 +349,29 @@ export function Link({
const route = parseRoute(url);
prefetchRoute(route);
startTransitionFn(async () => {
const newPath = url.pathname !== window.location.pathname;
window.history.pushState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
await changeRoute(route, {
shouldScroll: scroll ?? newPath,
unstable_startTransition: startTransitionFn,
});
const currentPath = window.location.pathname;
const newPath = url.pathname !== currentPath;
try {
await changeRoute(route, {
shouldScroll: scroll ?? newPath,
unstable_startTransition: startTransitionFn,
});
} catch (err) {
console.error('Error while navigating to new route:', err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have console.error here too.

throw err;
} finally {
if (window.location.pathname === currentPath) {
// Update history if it wasn't already updated
window.history.pushState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
}
}
});
}
};
Expand Down Expand Up @@ -464,15 +486,8 @@ const Redirect = ({ to, reset }: { to: string; reset: () => void }) => {
window.location.replace(to);
return;
}
const newPath = url.pathname !== window.location.pathname;
window.history.pushState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
const currentPath = window.location.pathname;
const newPath = url.pathname !== currentPath;
changeRoute(parseRoute(url), { shouldScroll: newPath })
.then(() => {
// FIXME: As we understand it, we should have a proper solution.
Expand All @@ -482,6 +497,18 @@ const Redirect = ({ to, reset }: { to: string; reset: () => void }) => {
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.catch((err) => {
console.log('Error while navigating to redirect:', err);
})
.finally(() => {
if (window.location.pathname === currentPath) {
window.history.replaceState(
{
...window.history.state,
waku_new_path: newPath,
},
'',
url,
);
}
});
}, [to, reset, changeRoute]);
return null;
Expand Down Expand Up @@ -596,6 +623,7 @@ const handleScroll = () => {
const InnerRouter = ({ initialRoute }: { initialRoute: RouteProps }) => {
const elementsPromise = useElementsPromise();
const [has404, setHas404] = useState(false);
const requestedRouteRef = useRef<RouteProps>(initialRoute);
const staticPathSetRef = useRef(new Set<string>());
const cachedIdSetRef = useRef(new Set<string>());
useEffect(() => {
Expand Down Expand Up @@ -662,11 +690,9 @@ const InnerRouter = ({ initialRoute }: { initialRoute: RouteProps }) => {
elements;
if (routeData) {
const [path, query] = routeData as [string, string];
// FIXME this check here seems ad-hoc (less readable code)
if (
window.location.pathname !== path ||
(!isStatic &&
window.location.search.replace(/^\?/, '') !== query)
requestedRouteRef.current.path !== path ||
(!isStatic && requestedRouteRef.current.query !== query)
) {
locationListeners.forEach((listener) =>
listener(path, query),
Expand Down Expand Up @@ -750,6 +776,7 @@ const InnerRouter = ({ initialRoute }: { initialRoute: RouteProps }) => {
const refetching = useRef<[onFinish?: () => void] | null>(null);
const changeRoute: ChangeRoute = useCallback(
async (route, options) => {
requestedRouteRef.current = route;
executeListeners('start', route);
const startTransitionFn =
options.unstable_startTransition || ((fn: TransitionFunction) => fn());
Expand Down Expand Up @@ -805,27 +832,28 @@ const InnerRouter = ({ initialRoute }: { initialRoute: RouteProps }) => {

useEffect(() => {
const callback = (path: string, query: string) => {
const fn = () => {
const fn = async () => {
const url = new URL(window.location.href);
url.pathname = path;
url.search = query;
url.hash = '';
if (path !== '/404') {
window.history.pushState(
{
...window.history.state,
waku_new_path: url.pathname !== window.location.pathname,
},
'',
url,
);
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,
);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use .finally here like the other use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

changeRoute(parseRoute(url), {
skipRefetch: true,
shouldScroll: false,
}).catch((err) => {
console.log('Error while navigating to new route:', err);
});
};
if (refetching.current) {
refetching.current.push(fn);
Expand Down
Loading