-
Notifications
You must be signed in to change notification settings - Fork 406
fix(examples): devinxi Tanstack Start example #2267
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
==========================================
- Coverage 77.05% 76.75% -0.30%
==========================================
Files 84 88 +4
Lines 2157 2491 +334
Branches 555 649 +94
==========================================
+ Hits 1662 1912 +250
- Misses 382 463 +81
- Partials 113 116 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I briefly checked the integration, and came across that global i18n instance used in some places eq The global i18n should be never ever used in server side rendered applications because all requests to the server would share the same context and using one global instance will cause a mess |
@timofei-iatsenko thanks for taking a look. What would you use instead of the global i18n instance? I'm not in production yet, but have not seen any particular issue so far in local env. |
@depsimon you will not see it in local development because you don't have a concurent access to your server-rendered app. You need to have a users which will access app at the same time and then you will get issues. The basic rule - you should create one i18n instance per request and share this instance everywhere. I'm not particulare familiar with tanstack start, so i could not help you with an example. |
I'm briefly checking the docs for tanstack start + tanstack router and probably Router Context + You might want to create i18n instance in before load hook and then pass it down using a RouterContext. |
Thanks for the pointers, I'll try to take a look at this before merging. It should be trivial to simply add the i18n instance to the router context in the It might be a bit trickier to pass it around in server routes (APIs). |
I pushed a commit which is getting rid of all global i18n instances usages. I think that the integration could be improved even more, instead of loading a catalogs using asynchronous import ( This will ensure that the loading of the dynamic catalog would follow the same invalidation / awaiting rules as other async recourses and you will not need handle loading state for catalogs separately. I think that the starting point might be Please check the code i pushed, because i didn't test all possible cases. |
Thanks @timofei-iatsenko I definitely missed the I updated the PR to remove unused imports & add a couple missing translations. The middleware is great, I used it in a project, the only downside is that this code is run twice for each API request that use the middleware: const locale = getLocaleFromRequest()
const i18n = setupI18n({})
await dynamicActivate(i18n, locale) You can attest this by adding a log in the
Interesting, we might achieve this in the |
that's fine, once es module loaded next time it will be taken from cache. it will not affect performance.
Yes i also was curios what is the recomended way to pass a hydration state from server to client. And was a bit disapointed when discovered that they are not showing how to do that and hide all logic in the |
@timofei-iatsenko I managed to make a simple router wrapper that handle the catalog hydration. I'm not sure how I did as I never played with that, but it seems to work. We could also delete the export function createRouter({ i18n: serverI18n }: { i18n?: I18n } = {}) {
const i18n = serverI18n ?? setupI18n({})
const router = routerWithLingui(createTanStackRouter({
routeTree,
context: {
i18n,
},
defaultErrorComponent: DefaultCatchBoundary,
defaultNotFoundComponent: () => <NotFound />,
scrollRestoration: true
}), i18n)
return router
} This way, only the server sets the locale & loads the initial catalog. Then it's loaded in the client. WDYT? |
So hydrate/dehydrate is the magic behind the I also don't like the fact that when you switch the language the catalog will be loaded in a different way. So imagine the situation:
I'm actually a big fun of the "stateless" as much as possible approach for the clientside applications. That means when you switch the langauge, i would assume that the whole page will reload from scratch. Because approach with switching language "on the fly" is requires many assumptions/testing and generally effort which is not really worth it. You need not only switch the translation of messages which is in the codebase, but also keep track that all resources will be reload on the correct language, if user in the middle of filling the form, all fields will be populated for correct language and so on. |
Yes, here's the I'd definitely need to be battle tested. It looked okay with all the examples in the example, but that's far from covering everything.
I think I agree with all of this. In this case I chose to loadAndActive + router.invalidate when switching the locale. To be honest that's what I usually do as it makes everything simpler at the cost of a reload, usually users don't expect to keep the current page as is when changing the locale, they just expect to be redirected to the same page in the correct locale. Though the form scenario you are talking about is already covered with the current state as long as the form values are identical (like the value of an option in a select). I'm not sure how we could handle loading the catalog in the server only as the SSR only happens once per session. We could probably return the locale + catalog from the |
I believe that localized version of the page should live in in its own path, according to http rules one URI should point to one resource, if you have FR and EN version on the same url ( It could be by domain Basically both versions are the same and difference would be only in middleware / reverse-proxy configuration magic for domain approach. With all above mentioned the app should have a "splat" route segment for a locale, and for this segment a usual "loader" for catalog could be implemented. So switching the language would be as easy as "<Link to="." params={{ locale: 'fr' }}>FR" Storing saved locale to the cookie could be kept, because this will improve user experience on subsequent visits if the language autodetect didn't work properly. |
The loader will run in the client when you click the Perhaps the URL-based locale detection can be left to the user's attention. I don't personally use that and there are probably many ways to approach that. In the end it's just yet another way to define the locale. |
Not really, it will always be loaded from the server on the navigation, there will be no call to any lingui methods other then navigating with a router. Anyway, what i'm trying to bring here, due to the fact that this example in the official repo, people will treat it as "official" way of integration. Sometimes just copying without wondering. So i believe it should showcase best practicies, not the dirty hacks. As i said the best practice to support SEO and caching and static site generation would be to have a separate path for each locale |
I've added an example page that uses a
Are you sure? In TSS, the loader is called client-side except for the first (SSR) request. Only the server functions are called in the server, but that wouldn't let us manipulate the client's i18n instance.
Do you think the In such a case, the |
@depsimon here is the code what i had in mind: // $lang/route.tsx
import { createFileRoute, notFound, Outlet } from "@tanstack/react-router"
import { updateLocale } from "~/functions/locale"
import { loadCatalog, locales } from "~/modules/lingui/i18n"
import { I18nProvider } from "@lingui/react"
import { setupI18n } from "@lingui/core"
export const Route = createFileRoute("/$lang")({
component: Page,
async loader({ context, params }) {
if (!Object.keys(locales).includes(params.lang)) {
throw notFound()
}
const messages = loadCatalog(params.lang)
await updateLocale({ data: params.lang }) // Persist the locale in the cookies
return messages
},
})
function Page() {
const messages = Route.useLoaderData()
const { lang } = Route.useParams()
const i18n = setupI18n({ messages: { [lang]: messages }, locale: lang })
return (
<I18nProvider i18n={i18n}>
<Outlet />
</I18nProvider>
)
} This route should be a parent of all routes. The loader indeed is isomorphic, so first time it's requesting data from SSR next time on client navigations it's executed on the client side. But this is OK, what i'm trying to achieve here is that initializtion and langauge switching is uniform. So instead of switching language in runtime and causing all components to re-render, we switch page and re-create i18n instance. In the current example not all routes is behind Also lingui has deps extrator feature, which allows to split catalogs by route. Than using loader will be the only one way to load these catalogs:
|
I think that if you setup a new instance in the Page component, it'll be different from the context.i18n which is the one you'd use in the loaders/head functions for instance. I'm not sure I understand why it's better to re-create a I18n instance rather than loading a catalog in the existing instance, is there a particular reason to that?
I don't want that for this example TBH. I just think there are many different strategies when it comes to how to structure your app in a i18n context, this is one valid way of doing it but I don't think it should be imposed on all users. That's why it's available in a simple form as a part of the example template (cfr the $lang/content route). If there is a demand, maybe it'll be worth to create two/multiple examples with different strategies (multi domain, subdomain, directory, ...). |
Description
This PR updates the tanstack-start example to use the latest version which relies on vite instead of vinxi. It also updates to tailwind 4
Types of changes
Could be fixing TanStack/router#4279 & TanStack/router#4409
Checklist