-
Notifications
You must be signed in to change notification settings - Fork 382
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
feat(react): introduce TransNoContext component for future RSC support #1738
feat(react): introduce TransNoContext component for future RSC support #1738
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
bafb45e
to
c8b94c5
Compare
const { i18n, defaultComponent } = useLingui() | ||
const { render, component, id, message, formats } = props | ||
const lingui = useLingui() | ||
return React.createElement(TransNoContext, { ...props, lingui }) |
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.
Should we also spread ...lingui
here?
It feels a little cleaner to pass the props directly rather than wrapping them 🤔
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.
there is a bit of mess with component
and defaultComponent
. It would look weird when they would be on the same level in props. I just wanted to save structure as it was in context.
return React.createElement(TransNoContext, { ...props, lingui }) | ||
} | ||
|
||
export function TransNoContext( |
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'm not very familiar with RSC yet.
Do we need a new component? Is this going to be part of the public api?
Can we distinguish RSC / client component at the useLingui
level so that the current Trans works for both? I think a new component is fine, just wondering.
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.
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 a new component? Is this going to be part of the public api?
Yes. Yes.
The new component might not be used directly, but as part of the nextjs integration. But because we could not predict all cases upfront it's better to make it as part of a public api. Having this, you can start directly using it in RSC and next app router without waiting for other changes to the lib or special integrations.
Can we distinguish RSC / client component at the useLingui level so that the current Trans works for both?
You don't have information is it executed in RSC or just SSR (client components) in runtime. Or i didn't find that.
The only way of detecting RSC i've found in the compile / webpack level. Which is not ideal. So some of the users may want to avoid compile time transformations and use NoContext version directly. For others we could create a nextjs plugin which would do a heavy lifting under the hood (see my explanation comment)
I'm opened for discussion / brainstorming how to support RSC better.
Description
Small step towards RSC support, more context in this comment: #1698 (comment)
Types of changes
Fixes # (issue)
Checklist