-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Add example app with React Intl #1055
Conversation
Let me know if there's anything you guys want changed. It would be great to get this merged. |
// Register React Intl's locale data for the user's locale in the browser. This | ||
// locale data was added to the page by `pages/_document.js`. This only happens | ||
// once, on initial page load in the browser. | ||
if (typeof window !== 'undefined' && window.ReactIntlLocaleData) { |
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.
Where is window.ReactIntlLocaleData
created?
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.
From the locale data modules; e.g. https://unpkg.com/[email protected]/locale-data/en.js
The server selects one React Intl's locale data scripts and inlines it on the page. Since the locale data is packaged as a UMD module, in the browser it creates the window.ReactIntlLocaleData
global.
More details here: https://github.com/yahoo/react-intl/wiki#locale-data-in-browsers
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.
Passing locale data around like this seems contrived 🤔 As I commented in formatjs/formatjs#731 (comment)
Is there no way for getInitialProps
to return the localeData and simply pass that to addLocaleData
?
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 are several issues with Next at play here:
-
page/_document.js
is bundled with Webpack, and errors our if you try to usefs
or any other built-in Node modules even though it only runs on the client. -
The
__NEXT_DATA__
is serialized usingJSON.stringify()
. This means thatgetInitialProps()
must return an object that's JSON-serializable, and React Intl's locale data isn't JSON-serializable since it contains functions for pluralization. -
Calling
addLocaleData()
on every page transition wouldn't be ideal. It's best to only call this function one time.
It's possible to address 1 and 2 of these issues in Next, and I'd be happy to rework example once they are changed, but as it stands now there isn't a good way around it.
For 2, serialize-javascript
can be used which does support serializing functions. Changing Next to use this instead of raw JSON.stringify()
would remove the need for this example to use a custom pages/_document.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.
Thanks for the details 🙂
- IMHO worth to create a bug on Next.js
- the localeDataScript itself (loaded by require.resolve + readFileSync) is just a string, which is serializable? Then the string could be evaluated where needed in the HOC (e.g with
eval
)? Right now the intl code is spread all around and that's hardly maintainable. - should work by returning a
serverRendered: !process.browser
in your getInitialProps and wrap theaddLocaleData
inif (props.serverRendered)
?: https://github.com/relatenow/relate/blob/ee51f1dd6990177ff0924b1c0d78fddfd23aa564/hocs/withIntl.js#L43
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.
how is using eval worse than dangerouslySetInnerHtml? Both sound ominous to me anyway. (but I'm no expert on this)
In this case dangerouslySetInnerHTML
is being used only on the server and it's just creating an inline <script>
in the HMTL response. Your idea of using eval()
would be running on the client would conflict with CSP; discussion here: #256
I hope not to spread i18n logic in my custom server etc just to handle this.
How are you doing HTTP content negotiation for choosing the locale on both the server and client?
I encourage people to who have apps that render on both the server and client to do the content negotiation and choose the correct locale server-side, and have the client use the locale the server decided on.
The main issue I ran into when trying to do this type of content negotiation and locale data loading in the HOC is that Webpack would bundle all locale data, all translated messages, and npm packages that were only meant to run on the client. I think the only way to keep everything in the HOC would involve i18n related stuff in a custom webpack config file — and I don't know what that would look like, but maybe you or others do?
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.
Thanks, I'm not familiar with CSP, will try to read more on this.
For detecting the browser locale I used express-request-language
, then store it in the session, and allow the user to customise it (server-side).
For co-locating the code a bit more, I just managed to load the message files from within the HOC (it does involve a slight webpack tweak): https://github.com/relatenow/relate/blob/master/hocs/withIntl.js#L25
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 just updated to use the accept-language
module to better co-locate the code: sedubois/relate@5c9d95a
This means that I don't need a custom server at all to use react-intl at this point.
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.
@sedubois do you want to send a PR to my branch with your suggested changes? Then I can merge your commits into this branch/PR.
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.
@ericf ok I can try to send something, maybe tomorrow (not sure if have time).
Are you guys interested in merging this? |
@ericf Sure are, Will have a review tonight/ tomorrow. Thanks for this PR, it's Great 🙌🏻🙌🏻 |
"version": "1.0.0", | ||
"description": "React Intl Next.js Example App", | ||
"author": "Eric Ferraiuolo <[email protected]>", | ||
"license": "BSD-3-Clause", |
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.
All examples have
name
the same as the directory in this casewith-react-intl
- No
description
- Empty author (
"author": ""
) - an
ISC
license https://github.com/zeit/next.js/blob/master/examples/basic-css/package.json#L15
Looks good, only added a comment for the package.json to keep it consistent. 👍 |
Looks good to me, LGTM after this |
@timneutkens @impronunciable okay I updated the |
Awesome ❤️ |
Was serialize-javascript issue fixed? Thank you. |
Fixes #1022