fix: locale switcher correctly works w/ localized Makeswift paths #2621
Conversation
🦋 Changeset detectedLatest commit: eb2d4e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
agurtovoy
changed the title
| // are used in combination with a given `pathname`. Since the two will | ||
| // always match for the current route, we can skip runtime checks. | ||
| { pathname, params, query: Object.fromEntries(searchParams.entries()) }, | ||
| { locale }, |
There was a problem hiding this comment.
The canonical way to handle this in next-intl is to use the pathnames configuration, but it's too inflexible for our purposes here:
- If we kept it as a build-time configuration, it would require a site redeploy after each Makeswift publish that updates page paths.
- A build-time configuration would also be incorrect for previews.
- If we converted it into a run-time configuration, we’d have to rework our code to retrieve the derived navigation components from a context and then cache and revalidate them at runtime.
The approach in this PR is more localized and arguably more performant, as we don't have to retrieve the whole sitemap.
| const localizedPathname = await getLocalizedPathname({ | ||
| pathname, | ||
| activeLocale: activeLocale?.id, | ||
| targetLocale: locale, | ||
| }); | ||
|
|
||
| startTransition(() => | ||
| router.push( | ||
| { | ||
| pathname: localizedPathname, | ||
| // @ts-expect-error -- TypeScript will validate that only known `params` | ||
| // are used in combination with a given `pathname`. Since the two will | ||
| // always match for the current route, we can skip runtime checks. | ||
| params, | ||
| query: Object.fromEntries(searchParams.entries()), | ||
| }, | ||
| { locale }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
My understanding is that route changes already trigger transitions. I'm curious why we're using it here.
Perhaps instead we meant to do this:
return useCallback(startTransition(async () => {
const localizedPathname = await getLocalizedPathname({})
router.push()
}))Note how the async transition will mark the transition as pending when the action itself is triggered. This pattern is actually what a "React Action" is, by definition.
There was a problem hiding this comment.
There is already a top-level startTransition call wrapping this callback here; the nested startTransition call follows the pattern explicitly set out for async transitions here: https://react.dev/reference/react/useTransition#react-doesnt-treat-my-state-update-after-await-as-a-transition.
That said, I didn't know that using router.push guarantees a startTransition call, which does make the explicit startTransition call unnecessary.
There was a problem hiding this comment.
Added a clarifying comment.
There was a problem hiding this comment.
A separate thought is: should the transition be handles outside of the hook, where it's not known whether there's a state update, or should it be handled inside as in my suggestion comment, so that the state update and transition are colocated 🤔
There was a problem hiding this comment.
I don't think there's a significant semantic difference in this case; you could argue it either way. I originally left it out of this particular hook and kept it in the LocaleSwitcher because, conceptually, switching the locale involves navigation, and that should be a transition, regardless of how the navigation is triggered.
There was a problem hiding this comment.
You could argue that because switching the locale involves navigation, the hook should encapsulate that and return the pending state together with the switchLocale callback. I'm sympathetic to that argument, but not enough to go rework this, haha.
There was a problem hiding this comment.
Yeah, I would argue the latter. And also, APIs like router.push encapsulate the transition as well.
That said, I'm not expecting you to rework this. Thanks for the fix!
There was a problem hiding this comment.
APIs like
router.pushencapsulate the transition as well.
But it doesn't return a pending state, so you have to wrap it in a startTransition call if you want that anyway. Similarly, the useSwitchLocale hook doesn't return a pending state because it was conceived as a lower-level primitive, and the pending state handling was left to the LocaleSwitcher component itself.
Now, if it occurred to me at the time, I'd likely go with the design you're advocating, I'm just pointing out that either of them is reasonable.
agurtovoy
force-pushed
the
fix/locale-switcher-makeswift-localized-paths
branch
from
4e86fdc to
9828a9e
Compare
agurtovoy
force-pushed
the
fix/locale-switcher-makeswift-localized-paths
branch
from
9828a9e to
5740bbc
Compare
agurtovoy
force-pushed
the
fix/locale-switcher-makeswift-localized-paths
branch
2 times, most recently
from
c0ca2d1 to
48aa68f
Compare
agurtovoy
force-pushed
the
fix/locale-switcher-makeswift-localized-paths
branch
from
48aa68f to
66a39fa
Compare
agurtovoy
force-pushed
the
fix/locale-switcher-makeswift-localized-paths
branch
from
66a39fa to
eb2d4e1
Compare
What/Why?
Fix locale switcher on localized Makeswift pages with different paths.
Testing
Kapture.2025-09-23.at.16.16.32.mp4
Migration
N/A