search

i18next / react-i18next

Internationalization for react done right. Using the i18next i18n ecosystem.
https://react.i18next.com
MIT License
9.2k stars 1.02k forks source link

Future of the NextJs example #609

Closed isaachinman closed 5 years ago

isaachinman commented 5 years ago

We can discuss the future of the NextJs example here, as it has gotten a lot of attention lately, and has some shortcomings.

Current problems:

  1. Language subpaths are causing a lot of issues
  2. Initial language and the translation HOCs are not working well together

Future goals:

  1. Refactor server and Link usage to match this example, thus preventing 404s and fixing #569
  2. Unify translation workflow, and make it less prone to user error: expose a single withNamespaces HOC that supports tree traversal, and make it abundantly clear that this is the only way to translate content. Fixes issues like #603.

I believe the best way forward is a complete rewrite to simplify a few things. The first step is to decide if we want to support language subpaths at all.

Are there any other issues people are aware of currently?

I would like to compile a full list of current issues we are facing here, as well as any new features, or features we plan to remove, before proceeding with any work on the NextJs example.

Only then can we have a clear plan and execute it.

isaachinman commented 5 years ago

@jamuhl Funny, as we were discussing privately - there is a with-react-i18next example in the NextJs repo. I see you contributed there as well. Is this supposed to be identical/synced with our example here, or different in some way? Should both examples exist simultaneously, or should we deprecate one in favour of the other?

jamuhl commented 5 years ago

@isaachinman the one on next.js is a lot older (v7) and i planned to update it as soon we got this one here stable (in the past we had just a readme here pointing to the sample on next.js) - but i decided to fix issues here - as i don't follow all the issues on next.js and think issues / questions related to it just pollute the next.js repo.

In the future i would prefer having a link from next.js to the sample here as it would make it easier to be maintained. But yet i'm not sure what the opinion of the next.js team is - as they seem to prefer having all the samples there??!

jamuhl commented 5 years ago

Important note: This is an example not a boilerplate.

Eventual one problem we have is we try to solve some stuff very convenient by eg. providing the withI18next HOC - which magically solves most needed to be done to work. My bet most people landing here with issues come from another example withApollo, withRedux, withMobx whatever and add the stuff shown in our sample which in most case just won't work without proper adjustments - so eventual we should more focus on showing what needs to be done to solve it (teaching) - instead of trying to hide away the more complicated things...but not completely sure - this is just something that came to my mind in the morning.

revskill10 commented 5 years ago

I agree with @jamuhl . Teaching users to understand how things work is the hardest and most important thing to be done. So more users could give help to issues, as well as contribute back to the library. Once users understand how things work basically, ones could customize themselves for their need.

Eruilz commented 5 years ago

Hi,

I've been testing this sample in my project for a few days, it is mostly working really great, so thank you ! I might have some input regarding a few issues :

Thank you again,

isaachinman commented 5 years ago

As a rudimentary way of voting, please thumbs up or down this comment to indicate whether or not you think language subpaths belong in the example.

Enalmada commented 5 years ago

Since your example is easier for you to maintain, you should link from nextjs examples to your example. I suspect the nextjs team will be fine with that since it reduces their maintenance and is a better user experience to see the most up to date example. There is some precedent for that here: https://github.com/zeit/next.js/tree/master/examples/page-transitions

Is there a way to detect broken ssr and throw exception in dev mode? It could be a good idea to consider doing that if it is possible so that people know something is not working as expected before shipping the product to production.

The example _app.js needs a comment linking to a page describing how to add i18next to a project already using something like Apollo. I tried adding i18next to my nextjs apollo project and could not fix the broken ssr despite reading all the relevant issues I could find only hinting I needed to merge something. I finally had to give up and tried adding react-intl example pattern to my apollo project and it worked fine in ssr. If you could try and add i18next to the nextjs with-apollo example and document exactly what it took to get it to work in all the closed "i18next with apollo" tickets that currently don't really contain any resolution, it would help a ton of people be able to use i18next rather than be forced to react-intl.

isaachinman commented 5 years ago

Hello again @Enalmada. Yes, we all agree the example should be hosted in this repo, and the NextJs team should link here. Just need to confirm that with @timneutkens.

Is there a way to detect broken ssr and throw exception in dev mode

React already does this for you. It'll let you know when the checksum doesn't match.

The example _app.js needs a comment linking to a page describing how to add i18next to a project already using something like Apollo

Couple things here:

  1. The new rewrite will hopefully not contain an _app.js file.
  2. This is a NextJs example. We are not doing anything else. No Apollo.

Please, let's stay on topic!

Enalmada commented 5 years ago

Hmmm...I have never seen a checksum error in the broken ssr situations I experienced. Is that something I need to enable or where would I see that? (I am very new to react so apologies if that is a silly question) If it will still be possible to have broken ssr after your enhancements to the example, just consider for a moment if there is something more that can be done in the next version of the example to prevent users from thinking their implementation works and shipping it to production when actually ssr is broken.

It is obviously on topic to call out ways the average user struggles to use the current example to add i18next to an existing project. I am sure you are already aware of the large number of apollo i18next issues, nextjs spectrum threads, stackoverflow links that exist without any solution to the problem. I am optimistic the future example will eliminate the specific problem people are currently having adding i18next to an apollo project, but if not, I am not asking you to add an apollo to the example, just that you consider a link to a solution in the readme to help people in that very common use case find a way through it.

Nextjs Link element also as some edge cases with i18next. You can see this thread here about it. I don't know if that is something that could be improved in the example code or if we need to talk with Nextjs team about it: https://spectrum.chat/?t=b9d1b95e-177b-4447-8cd1-bf2f488e44d5

Oh, and I would highly recommend creating a "next-i18next" project that would abstract out all code and sensible config defaults in example project that users are unlikely to need to ever modify themselves: like i18n.js, getAllNamespaces.js, location of languages, etc. Ideally to get started with next and react-i18next, new users would only need to run "npm -i next-i18next" and add absolute minimum to server.js to initialize with configuration overrides that differ from sensible defaults. Think how awesome it would be that critical fixes/features that you normally would make to the example repos would now automatically roll out to all users as a version update (rather than users having to watch the example repo for bug fixes that you make. Changes that might be critical but they will likely never know about them because they have copied over the original example project files and moved on).

isaachinman commented 5 years ago

@Enalmada I have written many times that SSR is an advanced topic, and we expect users to understand and be aware of this.

This issue is to discuss the future of the NextJs example. It has nothing to do with Apollo, nor anything to do with some next-i18next repo. Unless your comments are directly related to the example, please post them elsewhere.

Apollo, warnings about SSR breakage, and future projects are all well outside the scope of this example and will be marked as off topic if needed.

Enalmada commented 5 years ago

My comments are all directly related to the future of the example from my perspective...I get that we both come at things from very different angles. Adding some supporting documentation referencing common issues the examples choose not to cover would be so helpful...I am not sure why the hostility.

Please try and understand the value of abstracting out dependencies and code to make it easier for users to onboard and maintainers to roll out fixes to the "glue" code between react-i18next and consumer projects. This is a pattern that nextjs users are used to: next-seo, next-offline, etc each wrapping sensible defaults and dependencies of things. Doing this would make a huge difference to the way you structure a future example so I just wanted to put it out there as an idea before you started just in case.

isaachinman commented 5 years ago

For those that want to follow the development of the new example, it can be found here (temporarily):

https://github.com/isaachinman/react-i18next-with-nextjs

jamuhl commented 5 years ago

@Enalmada @isaachinman not read through the full thread -> but just my 50 cents on why it workes with react-intl without issues. => You bundle the translations with your applications - therefore is no async loading (namespace splitting) going on - all synchronous - all easy as piece of cake. Just add the translations in react-intl sample as a lazy import and see everything starts breaking.

So we could just remove i18next-xhr-backend, language-detection -> so we get free from even doing custom stuff in the server.js. All gets simple - no problems.

Just this sample is about adding i18next in production ready way - not make it dead simple for just doing getting started demos by oversimplify the real challenges of SSR and i18n. My problem is what i see in all the errors, issues and questions...those people using next.js have most time not the slightest idea what they are doing - how initialProps work because it's so damn easy to get started with one of the samples.

jamuhl commented 5 years ago

@isaachinman @Enalmada so i really would say the sample should be rather straight forward to learn what needs to be done - but integrations with all the apollo, mobx, redux, whatever needs to be solved in userland. Extending the react-i18next documentation with samples / hints for doing more complex stuff will be very welcome - but would go to far for the sample.

Edit: I have hopes we could make the v10 sample more expicit on what gets done and less magic -> so devs learn what needs to be done and not get a magic solution they do not really understand and therefore can't make the step including it into other eg. with-apollo sample.

isaachinman commented 5 years ago

@jamuhl Yes, we're on the same page and have been from the start. It's an example, not a boilerplate. We intentionally do not want to "abstract out code". That would defeat the purpose of an example.

The new example I'm working on should be simpler and more clear, and will contain a lot of comment-based documentation.

It seems like everyone (at least so far) thinks the language subpath functionality should remain in the example, so I will rewrite that as well.

isaachinman commented 5 years ago

The new example is coming along very nicely, and is in fact nearly done.

@jamuhl I've hit one interesting problem that I need your help with - something we did not even realise was occurring in the original example:

Let's assume we've got a namespace called contact-us that contains some content for the contact page. If a user arrives at the root URL which does not rely on the contact-us namespace, and then the NextJs app navigates to the contact page, the i18next-xhr-backend will indeed fetch the relevant contact-us namespace JSON file. This was clear in the original example.

However, NextJs has no idea about i18next translation JSON network requests, or any of that. The result is that the incoming route (in this case contact-us) will be displayed before the translation data is loaded over the network.

The place to block a page transition while waiting for data is inside the NextJs getInitialProps. We can await any promise. However, I cannot seem to find anything useful to await.

I tried i18n.hasResourceBundle and i18n.getResourceBundle, but both are sync functions that return information about data already in the store.

I tried polling i18n.hasResourceBundle in a loop, but it seems that i18next-xhr-backend doesn't actually start making network requests until the React tree that requires namespaces is rendered, so that won't work either.

In short, it seems we're going to have to trigger network requests manually. It should be trivial, assuming there is a way to do this. Is there?

isaachinman commented 5 years ago

@jamuhl I was able to come up with a hacky solution that requires one to initialise the i18next-xhr-backend via class construction so that we can have a reference to it and later invoke class methods:

await Promise.all(
  i18n.nsFromReactTree.map(ns => {
    if (!i18n.hasResourceBundle(lng, ns)) {
      return new Promise(resolve => {
        i18nextXHRBackend.read(lng, ns, (error, loadedNS) => {
          i18n.addResourceBundle(lng, ns, loadedNS);
          resolve();
        });
      });
    }
    return Promise.resolve();
  })
);

Hopefully you can understand this. It's a quick solution and is nasty nested code. Basically it's calling the read method on the in i18next-xhr-backend class instance manually, and then after receiving the JSON, manually calling i18n.addResourceBundle.

This works 100%. However, it's clearly a workaround and I feel it's bug prone and likely to break outright in the future.

Let me know if there's an official way to achieve this that I'm missing, or if you would suggest a different approach.

This is the final hurdle to releasing the new example.

jamuhl commented 5 years ago

that is very hacky and won't work in production -> as soon you got two components both needed that namespace it will load it twice. i18next handles resource loading very nicely by doing proper deduplication. => https://github.com/i18next/react-i18next/blob/master/src/NamespacesConsumer.js#L116 is like it is solved in the current version

The loadNamespace function has a callback that will be called when translations were already loaded or got loaded.

The render prop or hoc both can be set to wait before rendering their inner content or pass down a prop signaling readyness - if that helps.

isaachinman commented 5 years ago

Thanks, I'll take a look at loadNamespace tomorrow. I think I tried it, but quickly passed as I didn't realise it did not return a promise. That's good news though.

By the way, none of this is happening inside the HOC, and will take place once per page render - I've refactored things quite a bit.

jamuhl commented 5 years ago

@isaachinman just had a short look at the sample...looks promising -> like the way initialProps get set explicitly without hiding that magically like in the old hoc

isaachinman commented 5 years ago

So, the new example is now ready for review.

Some noteworthy points:

  1. In the end, it was necessary to use _app.js. I thought we could get away with just using _document.js, but it's a bit more complicated than that. The way _app.js is set up, most users should never have to touch/change anything for basic use cases.
  2. Related to _app.js, the new HOC approach has completed abandoned NamespacesConsumer in favour of withNamespaces, and the I18nextProvider.
  3. In fact, we could have gotten away with using withNamespaces and I18nextProvider directly out of react-i18next core, were it not for the need for our nsFromReactTree array, which keeps track of all required namespaces, given a React component tree - irrespective of whether components are NextJS pages or not (this is crucial).
  4. I also had to make modifications to our usage of Link. @timneutkens example was very helpful - we need to parameterise our route params and also pass an as argument to achieve true SPA with multi-langs. In the new example, you can switch languages in a fully-SPA manner without any hard reloads.

For anyone that has time, please do review the code, experiment with the app, and let me know about any issues you might find. I've put the example on a free Heroku dyno here (it can take some time to boot).

isaachinman commented 5 years ago

The basic use flow for the example goes:

  1. Leave _app.js alone unless you know what you're doing.
  2. Wrap any pages/components you need to translate with the withNamespaces HOC, from lib/withNamespaces.
  3. Make sure to use components/Link instead of importing Link from NextJs directly.

That's it! Everything else should be straightforward and development should be a more or less "normal" React experience.

jamuhl commented 5 years ago

@isaachinman looks good to me...one thing I ask myself shouldn't we just add that https://github.com/isaachinman/react-i18next-with-nextjs/blob/master/lib/withNamespaces.js#L23 to the current implementation in the main repo -> does not hurt and for v10 we planned the same to better support SSR out of the box.

isaachinman commented 5 years ago

@jamuhl Yes, that's exactly what I was thinking as I wrote it. It's just a matter of pushing into a simple array. The thing is, there are definitely use cases where users would want to clear that array - new route, etc.

For this example (and probably in real projects) I decided that clearing the array on a new route doesn't make much sense as it only applies to clientside navigation, and the browser is already going to have the previous-route's namespaces anyways.

Anyways, after getting to know the react-i18next library better, in my opinion I18nextProvider and withNamespaces is the best approach currently available, and nsFromReactTree works flawlessly if you stick to it. I'm not sure how it would work if people are mixing in NamespacesConsumer etc.

jamuhl commented 5 years ago

@isaachinman fun fact withNamespaces uses NamespacesConsumer: https://github.com/i18next/react-i18next/blob/master/src/withNamespaces.js#L33

So doing same nsFromReactTree in the sample could be done in consumer https://github.com/i18next/react-i18next/blob/master/src/NamespacesConsumer.js#L25 (just in the new way - not using reportNS)

isaachinman commented 5 years ago

Ah yes, I remember seeing that now. Must have slipped out of my head immediately.

As far as I can tell, the new example solves all open related bugs/issues.

@jamuhl Is there anyone else whose review you'd like to request before we think about merging?

jamuhl commented 5 years ago

@isaachinman guess we could merge it...and from there improve with the promise based i18next,...

isaachinman commented 5 years ago

I think the promise-based release would only affect this line of the NextJs example.

But sounds good, I will put this work into a PR soon.

isaachinman commented 5 years ago

613

Nelrohd commented 5 years ago

@isaachinman I'm not really a pro i18next config but I had to set fallbackLng to null because in production the front-end always switched back to the DEFAULT_LANGUAGE when the server side language was right. Otherwise, the new example looks great, thank you very much.

revskill10 commented 5 years ago

The last example failed on my project. Decided to rollback to old working version. I'm afraid of trying out new things here. Such a lengthly and hopeless hours of debugging without any chance to get things done. Breaking without any chance for you to fix it. It's this library now. I'm not sure...

isaachinman commented 5 years ago

@Nelrohd That sounds like a bug. Can you help me to reproduce that? If setting fallbackLng to null fixes your problem, that potentially sounds like an issue with i18next-browser-languagedetector.

isaachinman commented 5 years ago

@revskill10 I am sorry to hear you are frustrated. I understand - creating SSR React apps with localisation and code splitting is not an easy task.

We are doing our best to improve the example and help other developers as much as we can.

I will personally help you with your project if you'd like - just email me directly.

jamuhl commented 5 years ago

@isaachinman @Nelrohd could it be initialLanguage gets not passed down. But we really need some steps to reproduce.

@revskill10 sorry for the frustration - if the old system worked there is no need to update i would say. We just try to make the new sample more like a learning point - looking at the last issues related to next.js and react-i18next most cases where related to changes made and combining other next.js samples without the knowhow to "glue" things together -> the sample here is no boilerplate and no just put everything in at it will not break under any changes made.

isaachinman commented 5 years ago

@jamuhl Yes, agreed, and I want to take some responsibility for those issues too. @revskill10 That is why I took the time to do a deep dive and rewrite the example from scratch. I spent quite some time on it. I am very confident it is a big improvement over the last example. If you have time to check it out, you might be pleasantly surprised. Regardless, if you do want help or assistance getting things up and running, you can PM me.

revskill10 commented 5 years ago

@isaachinman @jamuhl Really appreciated all your efforts. A thank you is not enough. But we should stop all the pain one and for once by introducing a next-i18next package. I think you have enough deep knowledge to make such thing happen. Our goal is to minimize the interface to integrate/update to existing project. Why ? By introducing an independent package, we have more chance to test it for many use cases without breaking them before installing it. And more next.js discussion should not belong to this project, which is pure React package. If nextjs is too common, a monorepo style is enough in this case. My two cents.

isaachinman commented 5 years ago

@revskill10 I believe this is what @Enalmada was asking for as well. Now that we have discussed and completed the example here, maybe it is time to consider this.

What would a nextjs-i18next package be? There is way more going on than being able to simply do a yarn add nextjs-i18next and being "done". If you look through the new example, you will see how many areas we need to touch. I suppose a package could be possible, but we would need to create a HOC to wrap _app.js so that we have access to getInitialProps, as well as some other things.

Nelrohd commented 5 years ago

@isaachinman @jamuhl In the config.js

...
detection: {
  order: ['header', 'querystring'],
},
...

And set fallbackLng: DEFAULT_LANGUAGE,

In my case, my Google Chrome language settings is fr but DEFAULT_LANGUAGE is en. So what happens is:

  1. Reload the page cmd + R (back-end refresh)
  2. I see french translations
  3. It changes to english translations (not good, I should see french)

Both my i18n.language and initialLanguage say fr when I debug.

Let me know if you need more info

Nelrohd commented 5 years ago

Note that set fallbackLng to null solves the problem.

isaachinman commented 5 years ago

@Nelrohd We only have English and German content in the example. What do you mean you see French translations? Do you have a repo I can check out and reproduce this with?

isaachinman commented 5 years ago

@Nelrohd Regardless of that, why did you change the detection settings? That is most likely your issue. As it says in the codebase:

only change if you understand what you're changing

Eruilz commented 5 years ago

@isaachinman Thanks for the work, the new example, after some work to make it as I wished, is working allmost perfectly. small question : is it normal that the language change event is fired at every page change ? seems weird (maybe something I did wrong, I "glued" a few thing on top of i18n)

Nelrohd commented 5 years ago

@isaachinman I setup my own website following the example but I can't share the code. I changed the detection because it's not what I wanted. When I change my settings in my browser, I want it to change right away when I refresh, I'm not sure what the cookie settings brings as value, can you explain this please?

isaachinman commented 5 years ago

@Nelrohd Please open a separate issue if you believe it's a bug, or post your question on StackOverflow. It sounds like a custom-implementation concern.

Nelrohd commented 5 years ago

I won't open anything since the example works fine for me and I have no idea what to asks, I'm just giving you feedback. I can try to reproduce the issue with the example maybe?

isaachinman commented 5 years ago

@Eruilz That is a great point, I had not noticed that. I believe it's because initialLanguage is being set on a new page render.

@jamuhl Is this expected/normal? Should we avoid resetting initialLanguage on the clientside?

If so, should be an easy fix.

isaachinman commented 5 years ago

@Nelrohd If you can reproduce the issue with the example, that would definitely be a bug. In general, use of cookies is the most reliable method I've found of sharing lang state between client and server. It would make sense in your case that your client reverts to English, because it isn't reading headers.

Nelrohd commented 5 years ago

It's probably the cause, I guess not many people will change their chrome language settings.

isaachinman commented 5 years ago

It's irrespective of your browser settings.

Nelrohd commented 5 years ago

What do you mean? Google Chrome uses language settings to set the languages header