-
Notifications
You must be signed in to change notification settings - Fork 383
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
docs: React server components example and docs #1903
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
56d9c8d
to
44d644e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
==========================================
- Coverage 76.65% 76.57% -0.08%
==========================================
Files 81 82 +1
Lines 2090 2092 +2
Branches 533 534 +1
==========================================
Hits 1602 1602
- Misses 375 377 +2
Partials 113 113 ☔ View full report in Codecov by Sentry. |
Hi @fromthemills, thanks a lot for the contribution! I just read through the PR and have some suggestions:
|
i quickly check the example, here is my first impressions:
|
@thekip Would you like this under Tutorials or Guides?
True it is not picked up by the extractor. Any suggestions for how we will "recommend" to do server only translations? Just using the instance? like: i18n._('Learn') |
What about creating a separate Tutorial page? As described in #1876 |
Hello and thanks for the PR! I already have have a good idea of what I wanted to cover and I was planning to make it a little more comprehensive:
@andrii-bodnar actually the locale switcher, as far as I can tell, should demonstrate that a full page reload is not needed (why do you think it should use full page reload?). However, when this all adds up, I fear it could take a lot of time to get aligned using the standard loop of [write code -> get a review -> address review], as each step takes time on one side or the other. So the way I see it we can (1) do the loop, or (2) merge this into a side branch of this repo, and I could take it from there and iterate toward a version that addresses the above. That way also your time and commits are reflected in the repo history. Just a suggestion - happy to do it either way. Thank you! :) |
44d644e
to
eda630a
Compare
@thekip @andrii-bodnar I incorporated your feedback into the PR. Most notable changes:
@vonovak I would prefer to just merge this. I think it kind of covers most things. Maybe after the merge you can refine more? Future worksRight now for server components translations I use the t(i18n)`Home` Obviously this is not ideal but as @thekip mentioned import { TransServer } from '@lingui/macro'
import { setI18n } from '@lingui/react' And then maybe in a next version add some dynamic switching capabilities like the POC @thekip made Something to think about |
TY, this is much better, though I'd still prefer to have one example over two 🤔. edit: but I guess it's okay, sorry for showing up late with my comments. @fromthemills may I ask why you didn't use the server-side Trans, as you linked? TY :) btw the docs import TransNoContext but don't use it. (I'll do a more thorough review later). |
No worries. True... one example might be better. But the old example used
Nice catch, thank you. Removed them.
Good point. Mainly, I did not want to use the more "experimental" part of the POC with the I see possible future in that in a next version of Lingui React you might be able to setup a i18n "context" with Lingui in 2 ways:
Usage can than differ from client and server components // client components only
import { I18nProvider } from '@lingui/react'
import { Trans, t } from '@lingui/macro'
<I18nProvider> // sets client context and part of js bundle
{t`Hello`}
<Trans>Hello</Trans> // also part of js bundle
</I18nProvider> // client or server components
import { setI18n } from '@lingui/react/server'
import { Trans, t } from '@lingui/macro/server'
setI18n(...) // sets server context
{t`Hello`}
<Trans>Hello</Trans> // server only not part of js bundle After that we could than go one step further and create a new package called import { I18nProvider } from '@lingui/react'
import { setI18n } from '@lingui/react/server'
import { Trans, t } from '@lingui/macro/resolve'
setI18n(...) // sets server context
<I18nProvider> // sets client context
{t`Hello`}
<Trans>Hello</Trans> // Knows render context and switch to use "@lingui/react/server" or " '@lingui/react"
</I18nProvider> /** @type {import('next').NextConfig} */
module.exports = {
webpack: (config) => {
config.resolve.plugins.push(require('@lingui/rsc-webpack-resolver');
return config;
},
} |
eda630a
to
2b1f3a4
Compare
I recently realized that neither There are two ways to fix that, one is explicitly add |
I'll need to double-check but for me, it seemed that using the LinguiTransRscResolver worked fine with extraction 🤔, when importing like this: I think it's fine to document using
I didn't do any alterations there so not sure what you have in mind 🤔 It's true that I didn't use the From your examples:
Ideally we should only need
My take is: we shouldn't add |
or extract a separate package, specifically for nextjs. I think if we will go with the way of |
I agree adding this to Lingui itself might be to contrived... the APIs are to unstable ATM.
Yeah, also leaning more in this direction of // next.config.js
const { withLingui } = require('@lingui/nextjs')
// sets up swc plugin and resolver
module.exports = withLingui({
// Other Next.js configuration goes here
}); // page.js or layout.js
import { setI18n } from '@lingui/nextjs'
setI18n('en') The only thing I don't like about it is that you would not have access to the underlying primitives and you are relying on the resolver and swc plugin config (runtimeModules) to do the switching. Or maybe something like below is possible? I don't know... maybe to complicated. import { setI18n } from '@lingui/nextjs'
import { Trans, t } from '@lingui/nextjs/macro' // always uses get18n |
Firstly, guys, I want to say a big thank to all of you. I really appreciated that someone take contributions that seriously and willing to discuss and help. I'm lacking this "second opinion" for most of the time.
This thing is actually bothered me from the beginning. I used some non-documented internals of nextjs, which might be subject of change at any time. From other side the concept is neat and gives excellent DX. We can try to reach nextjs team and ask them to make this public or at least notify them that one of the libraries relaying on that and them what they think. Also, just to be on the same page want to share latest changes which may affect discussion here:
If the way with standardizing i'm thinking about a more radical solution, what if we allow a macro to process a user defined symbol? (this will not work with babel-macro-plugin, but will work in all other compilers, babel-plugin and swc) // config
macro: {
{
module: "@myApp/lingui-trans-rsc",
export: "TransRsc"
}
}
// will process
import {TransRsc} from "@myApp/lingui-trans-rsc"
<TransRsc>Hello</TransRsc>
// into
import {TransRsc} from "@myApp/lingui-trans-rsc"
<TransRsc id="<hash>" message="Hello"></TransRsc> This will unblock people to use macro with custom |
config.module.rules.push({ | ||
test: /\.po/, | ||
use: [ | ||
options.defaultLoaders.babel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this line? @lingui/loader should return a ready to use module for webpack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, your right it is not needed. Coppied it from an example.
} | ||
``` | ||
|
||
As you can see in the example above we slight modified the default `I18nProvider` with our own implementation. Here we pass the active message catalog as a prop. This is to avoid importing all catalogs inside the `I18nProvider` component itself and thus making them part of the client bundle. For the people who are wondering: Why we are not just passing an instance of `i18n` the original provider component? Server components can pass data to client components but the caveat is that this needs to be serializable data and `i18n` is not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we will go with "@lingui/nextjs" may be pre-made this provider in that package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... not 100% sure. One the one hand you would want the package to "batteries included" but you don't want to inflate the scope to much. Leaning more to "it should be included".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this point, I'm not for creating @lingui/nextjs
package. Let's lay the groundwork first, try to be as universal as possible and see later about adding framework-specific packages?
website/docs/tutorials/nextjs.md
Outdated
|
||
Finally an example of a locale switcher. | ||
|
||
```jsx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tsx
I've checked my PoC once again, and yes, the |
One more thing to consider is Turbopack, the |
Created an issue in NextJs repo vercel/next.js#64244 |
Love the project ❤️
Dang 🫠
We could either incorporate this into
I really like this approach @thekip I added your changes. Also took the liberty to update the current nextjs example to
How do we see this PR? Do we merge this as is? It currently covers the basics without to much of the experimental stuff. And as I said before just using client component We could wait on #1909, to include TransNoContext example? |
Long story short, we can use What i'm thinking right now is how to correctly implement it in lingui. We can expose Features to consider:
I want to prepare another PoC with changes I mentioned to prove the ideas. |
Tested, that works, let's discuss public API, how i see that:
I believe it would be a good idea to include However, Next.js app router behavior is extremely weird. While SSR layouts executed after pages, but context inside react tree is flowing from layout to pages (means layout components executed before pages) That means for us that react context provider in Layout will be propagated to pages, but If we will not merge provider with Both options have pros and cons. I didn't find a way where we can set |
Global |
Available in the v4.10.0 🚀 @fromthemills could you please consider the new features in the example and the article? There is an example mentioned here - #1914 |
@andrii-bodnar @thekip Will incorporate the new changes in the docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we're not done here yet, just wanted to leave a few comments for the next iteration. thank you for the work! :)
```js | ||
//next.config.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```js | |
//next.config.js | |
```js title="next.config.js" | |
And please do the same for other files
|
||
type Props = { | ||
locale: string | ||
messages?: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
messages can be types like this:
import { type Messages } from "@lingui/core"
children: React.ReactNode | ||
} | ||
|
||
export function I18nProvider({ locale, messages, ...props }: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this implementation:
export function LinguiClientProvider({
children,
initialLocale,
initialMessages,
}: {
children: React.ReactNode
initialLocale: string
initialMessages: Messages
}) {
const [i18n] = useState(() => {
return setupI18n({
locale: initialLocale,
messages: { [initialLocale]: initialMessages },
})
})
return <I18nProvider i18n={i18n}>{children}</I18nProvider>
}
with the above, depending on how users set it up, the provider might end up re-rendering and if that happens, new i18n
instance would be constructed, and all Trans
client components would re-render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use a useMemo
with [locale, messages]
deps?
I don't really understand why it's became initial
since, when language switched this provider would be called with new props
Since version 13+ Next.js allows you to use [server components](https://nextjs.org/docs/app/building-your-application/rendering/server-components) using the app directory. Lingui can easily be used with server components. There are however some considerations to be made so we can make optimal use of server components and it's capabilities. | ||
|
||
1. We want to avoid shipping to much js to the browser if it is not needed | ||
2. We want to be able to easily distinguish between client component translations (part of the js bundle) and server component translations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why? 🤔
} | ||
``` | ||
|
||
As you can see in the example above we slight modified the default `I18nProvider` with our own implementation. Here we pass the active message catalog as a prop. This is to avoid importing all catalogs inside the `I18nProvider` component itself and thus making them part of the client bundle. For the people who are wondering: Why we are not just passing an instance of `i18n` the original provider component? Server components can pass data to client components but the caveat is that this needs to be serializable data and `i18n` is not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this point, I'm not for creating @lingui/nextjs
package. Let's lay the groundwork first, try to be as universal as possible and see later about adding framework-specific packages?
if (locale === 'fr') { | ||
return fr | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you add more languages, this pattern grows not nice, I suggest using an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this point, I'm not for creating @lingui/nextjs package
This is no longer a proposition after discovering react-server
export condition.
(for some reason it will not give me an ability to left a reply under the original comment)
|
||
Since version 13+ Next.js allows you to use [server components](https://nextjs.org/docs/app/building-your-application/rendering/server-components) using the app directory. Lingui can easily be used with server components. There are however some considerations to be made so we can make optimal use of server components and it's capabilities. | ||
|
||
1. We want to avoid shipping to much js to the browser if it is not needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. We want to avoid shipping to much js to the browser if it is not needed | |
1. We want to avoid shipping too much js to the browser if it is not needed |
I also think that layouts are not executed for client-side route transitions, so basically I ended up wrapping all layouts and pages in |
Hi @fromthemills, do you need any help with finishing this PR? |
Hi! I'll be happy to take this over the finish line but I'm vacationing now, so I can look into this after 22/5, unless @fromthemills plans to do that 👍. |
Since there's no news on this, I'll get started on this soon, probably later this week. 👍 |
Description
Types of changes
Fixes # (issue)
Checklist