Skip to content
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

Confusion with nextjs example #397

Closed
kachkaev opened this issue Mar 6, 2018 · 7 comments
Closed

Confusion with nextjs example #397

kachkaev opened this issue Mar 6, 2018 · 7 comments

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Mar 6, 2018

I was playing with next.js + react-i18next + typescript this weekend and fell into a pretty unfortunate trap, which took a while to investigate. It is to do with the interface of i18n.js file.

Because i18n.js uses require & module.exports and then pages go for import i18n from '../i18n';, it's easy to get lost in what's being passed around. Because of this I ended up providing translate() hoc with a wrong i18n (i.e., a module object, not a class) here:

const Extended = translate(['home', 'common'], { i18n, wait: process.browser })(Home);

This happened to me partially because I was using import/export everywhere inside my TypeScript project and so quite a lot of time was lost 😅

I'm wondering if module.exports could be made more explicit in i18n.js to help others avoid the same confusion. I'd export three non-default things rather than just i18n (which as I turned out has got .default property, equal to I18n class 😮 ). These exports would be:

module.exports = {
  i18nInstance, // result of require('i18next') + manipulations like .use(XHR), to be used in server.js
  I18n: i18n.default, // Class to be used in translate hocs later
  getInitialProps, // a detached async function to be used in pages or withI18next hoc
}

Here is how a modified version of i18n.js looks in my TypeScript project: https://github.com/callthemonline/client-nextjs/blob/63d5d15cadd1879fd62e20d045f5c12a35d6406e/server/i18n.ts

WDYT of changing module exports @jamuhl? I'll be happy to submit a PR to the next js repo (because the example is a couple of commits ahead) and we'll then sync this repo's example too.

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2018

The problem i see is:

// commonjs (dist/commonjs)
const i18n = require('i18next'); // -> you get the i18next instance

// webpack (dist/es)
import i18n from 'i18next'; // -> you get the i18next instance

// typescript
import i18n from 'i18next'; // -> you get an object { default: i18nextInstance, ... }

Honestly if i would restart i would start using only named exports (just remove that default export as it is used non-consistent).

module.exports = {
  i18nInstance, // result of require('i18next') + manipulations like .use(XHR)
  I18n: i18n.default, // Class to be used in translate hocs later
  getInitialProps, // function, detached from i18n
}

I'm not sure if that solves it...i18nInstance is the correct thing to use in webpack, i18n.default will be undefined. For typescript you now can use the named export I18n...

?!?

@kachkaev
Copy link
Contributor Author

kachkaev commented Mar 6, 2018

I my i18n.ts I use import * as i18n from "i18next";, which works the same as const i18n = require('i18next'). Then I export { getInitialProps, I18n, i18nInstance }, which is compatible to module.exports = { ... }.

i18n.default will be undefined

That's right, but that gets solved by import { I18n } from '../i18n' or const { I18n } = require('../i18n') in other project files. The point of exporting three things in parallel is to later refer to them like this:

This makes it easier to track what depends on what and also removes a combination I got trapped by:

https://github.com/zeit/next.js/blob/dbb1b732a06e92f8730d26fa01dac4d53c744732/examples/with-react-i18next/i18n.js#L59

https://github.com/zeit/next.js/blob/dbb1b732a06e92f8730d26fa01dac4d53c744732/examples/with-react-i18next/lib/withI18next.js#L2

We are exporting x and are then importing x.default with the same name x. I missed this this important difference and simply replaced module.exports = i18n with export default i18n while moving i18n.js to TypeScript. Because I forgot to change import i18n from '.../i18n, I was importing a wrong object and it took a while to investigate why my translate hoc was throwing an exception. It was receiving an object instead of I18n class.

My point is that if we change what's being exported in the example's i18n.js, fewer people may get into the same trap because they won't have to deal with a hidden magic i18n.default property they do not know about. We also won't have to create i18n.getInitialProps, which is not very clean.

I'm happy to submit a PR to next.js to better describe what I mean.

@jamuhl
Copy link
Member

jamuhl commented Mar 6, 2018

agree...i would say change that export...as it does not break - it does not hurt ;)

@kachkaev
Copy link
Contributor Author

kachkaev commented Mar 7, 2018

➡️ vercel/next.js#3959

@kachkaev
Copy link
Contributor Author

kachkaev commented Mar 7, 2018

I've got one more little question left 😄

What's the best way to toggle debug in development/production? If i18n.js only worked on the server, I'd be possible to replace debug: true with something like debug: process.env.NODE_ENV === 'production'. But how to make this universal? 🤔

It'd be great if the example had debug: false in production so that people did not figure out how to do this themselves.

@jamuhl
Copy link
Member

jamuhl commented Mar 7, 2018

@kachkaev not so deep into next.js...from guessing i would think process.env.NODE_ENV exists on client too (like in create-react-app) but chance is my guessing is wrong ;)

@kachkaev
Copy link
Contributor Author

kachkaev commented Mar 7, 2018

You're right, process.env.NODE_ENV does exist on the client too! 🎉

vercel/next.js#3964

@kachkaev kachkaev closed this as completed Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants