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

Next.js 10 #869

Closed
martpie opened this issue Oct 27, 2020 · 153 comments
Closed

Next.js 10 #869

martpie opened this issue Oct 27, 2020 · 153 comments

Comments

@martpie
Copy link

martpie commented Oct 27, 2020

Is your feature request related to a problem? Please describe.

Next.js is just out, and introduces i18n routing primitives

https://nextjs.org/docs/advanced-features/i18n-routing

Describe the solution you'd like

I think a lot of the features of next-i18next can be dropped, so next-i18next really just focuses on the i18next integration.

Describe alternatives you've considered

n/a

Additional context

n/a

@isaachinman
Copy link
Contributor

Yep, have been waiting for it to land. We'll want to rewrite so that the main concern of next-i18next is handling data management for SSR apps.

Would be very happy to have input from collaborators!

@kachkaev
Copy link
Contributor

@isaachinman nice! Could you elaborate a bit on what you mean by data management in this context?

@isaachinman
Copy link
Contributor

So, the NextJs core as of v10 intends to take all responsibility for locale routing. That means we can remove a lot of functionality from next-i18next.

What NextJs v10 does not do, is handle any sort of actual data management for users.

In the case of full SSG apps, you can, in theory, get away with passing your localisation data as props per page, kind of like this example. However, that means you'll be sending all of your translation data down to every single page.

If i18n routing is being taken care of by NextJs internally, all that's left for a package like next-i18next to do is handle the management of actual translation content. This is basically a harmony/synchronisation of i18next backend packages, and NextJs opinionated patterns. For example, we'll still need to hook into NextJs Router events and load in necessary namespaces on route change, etc.

@mercteil
Copy link

mercteil commented Oct 28, 2020

Very exciting release specifically in regards to internationalization. I am looking forward to be able to use SSG or at least gerServerSideProps to be able to load namespacesRequired and to abandon getInitialProps. However the features I see provided by Vercal at this time are language detection an an early intl libs support.

Few things are still not clear to me:

Basically:

If i18n routing is being taken care of by NextJs internally, all that's left for a package like next-i18next to do is handle the management of actual translation content. This is basically a harmony/synchronisation of i18next backend packages, and NextJs opinionated patterns. For example, we'll still need to hook into NextJs Router events and load in necessary namespaces on route change, etc.

@SalahAdDin
Copy link

Does NextJS handle localization files? If it is using react-i18n and i18n-next it is still possible to use their hook to handle switching languages.

@isaachinman
Copy link
Contributor

NextJs only handles the routing. Handling localisation files, splitting translations across pages, etc, will be the new goal of packages like next-i18next.

I hope to spend some hours this weekend taking a deep look at a rewrite for next-i18next to simplify things dramatically.

@underfisk

This comment has been minimized.

@MathiasKandelborg
Copy link
Contributor

MathiasKandelborg commented Oct 31, 2020

@isaachinman Lmk if I can help. I've done some things to load translations specified with namespaces, with SSG.

https://github.com/MathiasKandelborg/TRUE-framework/tree/master/util/i18n

I have time to contribute so I might take a look myself. Would love to contribute, so let me know if there's anything I can begin with.

@isaachinman
Copy link
Contributor

@MathiasKandelborg I am starting to look into this now, but unfortunately only have about an hour today.

Overview

I will write my thoughts below, to give any potential contributors a chance to work on this without my schedule blocking development. I understand that this is time-sensitive in a way, and would greatly appreciate help anyone might be able to offer.

In general, the future version of next-i18next should be a dramatically simpler codebase to maintain and reason about.

It should be said that NextJs's i18n routing implementation has less features than next-i18next's current implementation. Whether or not that is a bad thing is entirely subjective, it's just down to certain decisions the Vercel team has made. For example, if people want to enforce locale subpaths, they will need to write their own redirects in next.config.js, etc.

As I wrote above, the main purpose of next-i18next moving forward will be the handling of namespaces, i18next-fs-backend, I18nextProvider, and so on.

Software Approach

The next-i18next package will just export functions now, we don't have a need for any internal state, class construction, any routing logic, any i18next middlewares, etc. The _app HOC should take the config, like so:

User's custom _app:

import { appWithTranslation } from 'next-i18next/dist/commonjs/hocs/app-with-translation'
import path from 'path'

const MyApp = ({ Component, pageProps }) => <Component {...pageProps} />

export default appWithTranslation(MyApp, {
  defaultLanguage: 'en',
  otherLanguages: ['de'],
  localePath: path.resolve('./public/static/locales')
})

Individual pages should continue passing namespacesRequired up to the HOC via props, but can do so via either getStaticProps or getServerSideProps – either way works agnostically to next-i18next, as it just comes up as WrappedComponent.props:

User's homepage:

const Homepage = ({ t }) => (
  <Link href='/second-page'>
    <button
      type='button'
    >
      {t('to-second-page')}
    </button>
  </Link>
)

export const getStaticProps = async () => ({
  props: {
    namespacesRequired: ['common', 'footer'],
  }
})

export default withTranslation('common')(Homepage)

The app-with-translation HOC itself can now just consume the locale via router.locale.

app-with-translation HOC:

export const appWithTranslation = (WrappedComponent, userConfig) => {

  const WrappedComponentWithSSR = withSSR()(WrappedComponent)
  const config = createConfig(userConfig)
  const { initPromise, i18n } = createClient(config)

  const AppWithTranslation = (props) => {
    const router = useRouter()
    i18n.changeLanguage(router.locale)
    return (
      <I18nextProvider
        i18n={i18n}
      >
        <WrappedComponentWithSSR
          {...props}
        />
      </I18nextProvider>
    )
  }

  return hoistNonReactStatics(
    AppWithTranslation, WrappedComponent,
  )
}

Things I haven't figured out yet:

  1. How to await the i18next init promise. Do we need to wrap getStaticProps/getServerSideProps, to be able to await the promise? Probably so...
  2. How to properly configure i18next in the absence of i18next-http-middleware. Just from an initial look, it seems like that package was doing a lot of work for us for free, in terms of configuration

Call for Contributors

That's it, really. I don't think it should be more than a few days of work to strip next-i18next down to a much slimmer, simpler package that is agnostic of routing.

I would greatly appreciate the help of anyone who wants to work on this. If anyone wants to dedicate a good chunk of time, let me know and I will consider adding people as contributors, so that we can collaborate on a next-v10 branch, or something similar.

I'd also greatly appreciate advice and feedback from @adrai and @jamuhl, as it looks like the future of next-i18next is basically just a thin i18next wrapper on top of NextJs.

@SalahAdDin
Copy link

Why a HOC? Wouldn't it better to use hooks instead?

@underfisk
Copy link

@SalahAdDin What about react legacy apps? If it goes only for hooks, they lose support because they have no functional approach as such

@kachkaev
Copy link
Contributor

kachkaev commented Nov 1, 2020

Apps with React 16.8.0+ support hooks, which means that any project using React 16.0.0+ (Sep 2017) can upgrade without any breaking changes.

React 16.8.0 is a minimum requirement for next-i18next already:

https://github.com/isaachinman/next-i18next/blob/abdf06545410f340b0529e3448f8b102ab840249/package.json#L115-L118

@underfisk
Copy link

@kachkaev Okay then it would make sense

@jansepke
Copy link

jansepke commented Nov 1, 2020

@kachkaev but still many have not migrated class components to functional components. next-i18next currently supports useTranslation and withTranslation already so you can choose between HOC or hook usage (but maybe its not documented).

If you mean the appWithTranslation HOC: afaik hooks cannot insert a provider block around your app

@underfisk
Copy link

@jansepke but useTranslation does not work, i have tried and it doesn't, so you are required to receive the TFunction. Its weird because i18 should have the loaded translations but if you try to useTranslation, the translations are empty (Right now with my hooks app i'm forced to use the T everywhere with HoC because next-i18next does not inject correctly)

@mercteil
Copy link

mercteil commented Nov 1, 2020

@jansepke hooks are not for component wrapping. But it is still just a function. You could use any function to be a factory. However we would not use hooks in the react sense anymore. Thus we use Higher Order Components(=Factory Functions). However if you really need to you can return a Component from a hook and thus wrap it within a hook. For next.js => SSR does not render hooks. They are executed on the client side (see useEffect eg).

@underfisk I use useTranslation application wide. Your configuration seems to be falsy.
Anyhow, better to open an issue and not communicate it under this topic.

@itxtoledo

This comment has been minimized.

@underfisk

This comment has been minimized.

@isaachinman
Copy link
Contributor

We'll be using a HOC, let's keep discussion in this issue on topic.

@underfisk

This comment has been minimized.

@kachkaev
Copy link
Contributor

kachkaev commented Nov 4, 2020

How to await the i18next init promise

What about throwing a promise in one of the HOCs and thus leveraging the Suspense mechanism? 🤔

@isaachinman
Copy link
Contributor

@kachkaev That's an experimental feature, and not really relevant what we want to do. The moments where we will need to await the init promise are either during SSG or SSR. A fully initialised i18next instance is serialised and sent down to the client per req, so there is no need to await in a client side setting.

@MathiasKandelborg
Copy link
Contributor

This looks promising! 🎉 Let's get some things going! 🥳

The project's new scope is "simply" integrating i18next with Next.js, right?

Would it be fine for a first PR to try and cut as much as possible, get all the way down to the bone, and then get started with Next.js v10 and i18next?

I think a project or a milestone (on this Github repo) with some issues etc. would help organize things a bit better. It might just be me liking to organize things a bit too much, though.


There are several approaches to be made in regards to configuration and file loading. I'll spend some time today looking at different ways to approach the integration.

I found an approach - i.e., the files I've linked to before - to include the translated files on build/compile time for SSG pages using a Next.js lifecycle (haven't tested others, but the concept should be the same).
This approach loads the relevant namespace files before any requests are made, so the data is included in the initial request.
If it's possible to use the aforementioned method with i18next and all the different rendering approaches, that will probably be superior to any other method I can think of. I'm not sure about it, though. I'm just putting some thoughts out there.


BTW, sorry for the late response!

I've had a hectic week so far; I'm going to spend at least a few hours a day trying to solve this. I can't really progress my own project as I want to unless I use a library like this, so it's getting personal now! 😁

@isaachinman
Copy link
Contributor

Hey @MathiasKandelborg.

Would it be fine for a first PR to try and cut as much as possible, get all the way down to the bone, and then get started with Next.js v10 and i18next?

Not sure that's necessary – if you want to focus on something, let's begin with the core functionality. I have the context necessary to quickly clean up the repo once the core work is done.

I found an approach - i.e., the files I've linked to before - to include the translated files on build/compile time for SSG pages using a Next.js lifecycle

Can you explain what you mean? Loading translation files is something that next-i18next has already solved, and I am not anticipating any changes will be needed in regards to that.

Also, I will have at least a few hours to look into this tomorrow (8 Nov), so let me know if you make any progress in the meantime.

@isaachinman
Copy link
Contributor

Pretty crucial problem: _app does not support getStaticProps or getServerSideProps. I'm unsure how to write a HOC that adds an i18n instance, as we need to init it during SSR/SSG, and then serialise it via pageProps down to the client. Using getInitialProps seems like a step backward, as these are static data requirements.

Attempting to override, or compose upon getStaticProps or getServerSideProps manually would be tricky or impossible, as I'm not sure we can access pathname.

Would be grateful for any suggestions.

@kachkaev
Copy link
Contributor

kachkaev commented Nov 8, 2020

What if getXyzProps returns the actual translations and not paths / namespaces? Roughly speaking,

export const getServerSideProps = async () => { // or getStaticProps
  return {
    props: {
      translationLookup: loadI18nextTranslations(["myPage", "whatever"]), // never executed on the client
    },
  };
};

const MyPage = ({ translationLookup }) => {
  return (
    <NextI18nextProvider translationLookup={translationLookup}>
      pageContents
    </NextI18nextProvider>
  );
};

export default MyPage;

The problem with this though is that if every page has loadI18nextTranslations(..., "common"), then common translations would be loaded on every transition to another page. Perhaps, they could be some special trick for those via _document.tsx? What I mean is preloading common namespaces there and thus not having to include them into the pages.

@kachkaev
Copy link
Contributor

kachkaev commented Nov 8, 2020

An alternative solution would involve an API endpoint and Suspence inside some sort of <NextI18nextProvider namespaces=["foo", "bar"]/>. If the instance of i18next inside the NextI18nextProvider does not have the expected locales loaded, it throws a promise, which resolves after the API request has been fulfilled. What I mean here is calling something like /api/next-i18next-translations?locale=ab-CD&namespaces=commons,myPage.

In addition, each use of a not-yet-loaded namespace via t / Trans can also trigger Suspense, but show a warning. The warning will help the devs predefine the locales they expect to be used at the top of each page and thus make the app faster. Still, because of how Suspense works, the app will be functional even if a requested namespace is not yet loaded in the middle of a page.

@nowshadfriday
Copy link

Hey guys, I was trying to follow the example for my NextJS app, but was facing a problem when setting up a dynamic page like below - pages/steps/[slug].ts and depending on the slug I'm showing different components.
Is it possible to include an example of such case? https://github.com/isaachinman/next-i18next-vercel @isaachinman

@filipesmedeiros
Copy link
Contributor

@nowshadfriday I think you just have to access the params prop (in your getServerSide/StaticProps) and based on that fetch the translations you need. next-i18next only cares about the keys and languages you want.

@yovanoc
Copy link

yovanoc commented Feb 5, 2021

Maybe related to this? #919

@zalcode
Copy link

zalcode commented Feb 11, 2021

Error: Error serializing ._nextI18Next.userConfig.interpolation.format returned from getStaticProps in "/about".
Reason: function cannot be serialized as JSON. Please only return JSON serializable data types.

I can't define interpolation.format option in 8.0.0-beta.4.

@muserref-sefer
Copy link

Can you give a simple example? @isaachinman

@isaachinman
Copy link
Contributor

Serialisation of functions is an open issue, and is yet to be solved, although there have been a couple helpful suggestions.

@msefer There is a simple example on the next-v10 branch.

@isaachinman
Copy link
Contributor

So, I am proposing to solve the issue of serialisation by using serialize-javascript, thus side-stepping NextJs' strict serialisation implementation.

Please checkout #954 and #931 for more info.

@filipesmedeiros
Copy link
Contributor

Btw @isaachinman I don't know if this has been addressed in this thread but I think not. How should apps handle common translations that are needed in _app (like for layouts)? Should the pages be responsible for requesting the namespace that the _app needs?

@isaachinman
Copy link
Contributor

@filipesmedeiros Namespaces get passed into the second argument of serverSideTranslations – how you organise translations is up to you. Based on what you've just described, yes, you should probably just drop those keys into a common namespace.

@filipesmedeiros
Copy link
Contributor

filipesmedeiros commented Feb 21, 2021

But we would have to explicitly pass 'common' on every page, correct (or make an app-land high order function to do it for us)?

@isaachinman
Copy link
Contributor

Yes. That's not something I would be looking to optimise for, at least not for the initial release.

@isaachinman
Copy link
Contributor

isaachinman commented Feb 21, 2021

Update:

  1. I've solved serialisation via serialize-javascript
  2. I've modified the API and configuration slightly (new setup below)
  3. I've added e2e coverage via Cypress, and increased unit test coverage to 96% on next-i18next@beta

I will most likely release next-i18next@beta as [email protected] sometime next week.

I imagine there might be some rough edges, but in general, migration of existing projects should be pretty straightforward.

Here's the new setup:

First, create a next-i18next.config.js file in the root of your project. The syntax for the nested i18n object comes from NextJs directly.

next-i18next.config.js

module.exports = {
  i18n: {
    defaultLocale: 'en',
    locales: ['en', 'de'],
  },
}

Now, create or modify your next.config.js file.

next.config.js

This tells next-i18next what your defaultLocale and other locales are, so that it can preload translations on the server, etc.

Second, simply pass the i18n object into your next.config.js file, to enable localised URL routing:

const { i18n } = require('./next-i18next.config')

module.exports = {
  i18n,
}

For full docs, check out the beta README. The latest work has been released under [email protected]. I'm fairly sure the API is stable now.

@LeonardDrs
Copy link
Contributor

Hey @isaachinman thanks a lot for this!
I've noted that the modifications from #934 are not ported to the beta.

Would you like a PR to do so?

@isaachinman
Copy link
Contributor

@LeonardDrs Yes, that would be great, thanks!

@LeonardDrs
Copy link
Contributor

Okay, doing it right now.
Also sie-note about the new modifications to the config file handling:
This means that all our custom config parameters for next-i18next are also fed to the next.i18n configuration:

next-i18next.config.js

module.exports = {
  i18n: {
    locales: availableLanguages,
    defaultLocale: "fr-FR",
    localePath: path.resolve("some/path/to/locales"),
    localeStructure: "{{lng}}",
    nsSeparator: ":::",
    keySeparator: "::",
    postProcess: "sprintf",
    use: [sprintf],
    ns: "common",
  },
};

next.config.js

const { i18n } = require('./next-i18next.config')

module.exports = {
  i18n, // This includes also next-i18next custom parameters
}

In this case, should we use this instead ?

const { i18n } = require('./next-i18next.config')

module.exports = {
  i18n: {
      defaultLocale: i18n.defaultLocale,
      locales: i18n.locales,
  },
}

I feel like it's worth mentioning somewhere

@isaachinman
Copy link
Contributor

isaachinman commented Feb 22, 2021

@LeonardDrs No that's incorrect, and definitely a miscommunication on my part. Your setup would look like:

next-i18next.config.js

module.exports = {
  i18n: {
    locales: availableLanguages,
    defaultLocale: "fr-FR",
  },
  localePath: path.resolve("some/path/to/locales"),
  localeStructure: "{{lng}}",
  nsSeparator: ":::",
  keySeparator: "::",
  postProcess: "sprintf",
  use: [sprintf],
  ns: "common",
};

As in, the i18n nested object only contains NextJs specific configuration. We actually destructure it out internally anyways. This is all just to provide an easier way of reusing config for users.

Moreover, I'm fairly sure NextJs will throw if you pass unknown config options into it.

@janhesters
Copy link

Worth mentioning, you no longer need static/ folder but instead use public/locales/. Was stuck on this for 20 minutes when upgrading from beta.6 to beta.8 lol.

@isaachinman
Copy link
Contributor

@janhesters Great point, and sorry for catching you on that.

Open request: would anyone be willing to write a migration guide, once we've finalised the API? I think it would be more helpful if this came from an actual user, with an actual project.

@isaachinman
Copy link
Contributor

Hi all, I believe [email protected] is just about ready to go: #595.

If anyone has a free moment, would you mind reviewing the new README? I am expecting to receive a number of issues immediately following release, and that's fine with a major version like this, but I would like to at least get the documentation correct from the start.

@LuudJanssen
Copy link

LuudJanssen commented Feb 24, 2021

Thank you for your time @isaachinman! Very excited for the 8.0.0 release. Heads up: We didn't get to it yet, but we'll migrate one of our biggest clients to from version 7 to built-in Next.js locale routing + version 8 in the upcoming two weeks. I'll take the time to closely review the new documentation and provide issues and pull requests for any things we'll find out along the way.

Thank you and all of the other contributors very much for the plugin and taking the time to support Next.js version 10!

@LeonardDrs
Copy link
Contributor

LeonardDrs commented Feb 24, 2021

I find the README good.
There is a paragraph present twice, the one about `useTranslation hook:

You can use NextI18Next.useTranslation hook too! See in react-i18next docs

     // This is our initialised `NextI18Next` instance
     import { useTranslation } from '../i18n'
     
     ....

There are three functions that next-i18next exports, which you will need to use to translate your project:

If we still want to preserve this part, we should edit the from '../i18n' taken from the example to from 'next-i18next'

@isaachinman
Copy link
Contributor

Good catch @LeonardDrs – I think that was a result of the merge from master.

@isaachinman
Copy link
Contributor

I have published [email protected] to npm, and moved next-i18next.com to Vercel.

Thanks everyone!

@michchan
Copy link

I'm still not able to get it work with next export with the setup instructed.
This is my workaround #10 (comment)

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