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

FEEDBACK WANTED FOR NEXT MAJOR VERSION: using HOOKS and SUSPENSE #590

Closed
jamuhl opened this issue Oct 28, 2018 · 24 comments
Closed

FEEDBACK WANTED FOR NEXT MAJOR VERSION: using HOOKS and SUSPENSE #590

jamuhl opened this issue Oct 28, 2018 · 24 comments

Comments

@jamuhl
Copy link
Member

jamuhl commented Oct 28, 2018

We are rather amazed at how much smaller and easier react-i18next feels using hooks and suspense.

=> Early Feedback would be very welcome 🙏

A little introduction: https://react.i18next.com/experimental/using-with-hooks

sample code: https://github.com/i18next/react-i18next/tree/master/example/react-hooks

new source code: https://github.com/i18next/react-i18next/tree/master/src/hooks

@schettino
Copy link
Collaborator

at first glance, it seems very straightforward. I'd say, however, to use useTranslation instead of useT. Different from the t, which is enough to express what we want, useT might lack a little of that expressiveness, what do you think?

@jamuhl
Copy link
Member Author

jamuhl commented Oct 29, 2018

@schettino totally agree will update that on the next release to useTranslation and withTranslation - no other hook use cryptic short namings - so we shouldn't do either

@isaachinman
Copy link
Contributor

Again taking a quick glance - this looks great. Is it fair to say this is a 1:1 rewrite, using hooks, without any other kind of refactoring? There are some things going on I don't understand (like throwing a Promise), but the actual usage of hooks looks correct to me.

Is it true that you can't pass functions into useState? That seems like a bug to me if it's true. You tried binding your args and passing the function reference itself?

@jamuhl
Copy link
Member Author

jamuhl commented Oct 30, 2018

Yes, not being able to pass a function, dan already mentioned that this is a problem on twitter - i guess that will be sorted out at some point (for now it's an "acceptable" workaround).

Yes, there is not much functionality removed (beside not using context api, removing the render prop - which could easily be added in userland if needed) - just need to figure out the SSR part again. Not yet sure if by adding an explicit hook, Component or by just passing a second argument to the useT hook containing the props (initialStore, initialLanguage). reportNS also is not in right now.

@jamuhl
Copy link
Member Author

jamuhl commented Oct 30, 2018

@isaachinman throwing the Promise -> that is how Suspense works under the hood...it catches and if it's a promise -> showing the fallback. When all promises resolve it goes back and renders the actual content...a little weird but seems to work 🙄

@isaachinman
Copy link
Contributor

Ah, I didn't see that tweet. As long as they're aware of it, and are treating it like a bug, no problem. Also strange to hear that about suspense. I watched Dan's talk awhile back but haven't gotten hands-on with it yet. Throwing a promise seems like a really weird API, but I guess good to know!

Maybe you can explain a bit more about what we need to get SSR working again? In my opinion a minor-bump-refactor is not worth it if it means dropping crucial functionality like SSR. Is it your plan to get SSR working before merging this? It sounds like you have a few possible approaches in mind. We can discuss if it is helpful to you.

@jamuhl
Copy link
Member Author

jamuhl commented Oct 30, 2018

@isaachinman not yet sure if there also will be some overhaul on next.js itself...but for sure SSR will need to work for v10.

guess reportNS could be implemented as a "global" variable inside the hook - so we fill that under the hood without extra pass reportNS -> and expose that on i18n or similar. Than it's just about handling the initialStore/Lnaguage which in my opinion should be done expicit in a separate SSR hook or component.

@isaachinman
Copy link
Contributor

Are we discussing SSR with NextJs, or in general? I don't really understand your point about reportNS, why does that implementation need to change?

@jamuhl
Copy link
Member Author

jamuhl commented Nov 1, 2018

both next.js will be the focus...but should be helpful on SSR using razzle or whatever too: -> https://github.com/i18next/react-i18next/blob/master/src/hooks/context.js#L59 is usable tor razzle too (same for the withSSR hoc)

@Jessidhia
Copy link

Jessidhia commented Nov 9, 2018

A complication I notice when trying out both hooks and suspense at the same time is that suspending a component will throw out the work in progress hooks.

If you want to suspend as part of implementing wait, for example, the Promise can't be created or stored in the component's own state. I suspect that is a bug but I haven't seen it confirmed either way.

Based on recent commit activity I've seen it's possible it's a synchronous mode only issue as well, but I haven't tried asynchronous mode.


What I mean is, mounting this component will enter an infinite loop, always recreating the ref and the promise on every resume because their work in progress state was discarded:

function UsesResource(props) {
  const resourceRef = useRef(undefined)
  const resourcePromise = useMemo(async () => {
    resourceRef.current = await import('./resource')
  }, [])
  if (resourceRef.current === undefined) {
    throw resourcePromise
  }
  render <>{/* stuff using resourceRef */}</>
}

@jamuhl
Copy link
Member Author

jamuhl commented Nov 9, 2018

@Kovensky there is nothing like work in progress for i18next -> it get's triggered to load and stores all translations in it's own "store" -> so even discarding a component won't stop loading and won't trigger a reload.

the hook works like:

call the hook first time: are needed namespaces loaded?

-> yes: go on and return values
-> no: throw the promise triggering suspense (on load done callback resolve that promise)

@Jessidhia
Copy link

Ah, right, you can avoid the problem by not having the thing storing the data you care about being inside this component 🤔

@vict-shevchenko
Copy link

Hello Jan.

What do you think about switching for a full name also for such case:

import { initReactI18n } from 'react-i18next/hooks';

Using initReactI18next seems more convenient than initReactI18n to me.

Actually, in my project, I am using full name i18next in all places and not shortcutting it to 'i18n', not to confuse with other projects like https://github.com/blacktangent/react-i18n.

@vict-shevchenko
Copy link

Jan,

Excuse me if I will tell a nonsense now, but seems current implementation of hooks does not cover such weird scenario (when different parts of the application are localized with different instances of i18n):

import i18nextA from './i18nextA.js' // maybe different backends
import i18nextB from './i18nextB.js'

...

return (
<Fragment>
  <I18nextProvider i18n={i18nextA}>
    <FirstApp />
  </I18nextProvider>
  <I18nextProvider i18n={i18nextB}>
    <SecondApp />
  </I18nextProvider>
</Fragment>
)

I understand that keeping I18nextProvider HOC is an additional code in the project, but this will allow next few things:

  • different instances of i18next in the same application
  • storing and passing i18next instance will be done via React context, so I thought that code that stores i18next for the application (initReactI18n) can be removed? Feels like we are reimplementing context with it now.
  • fetching of a proper instance of i18next for useTranslation hook can be done based on useContext native React hook
  • keeping import './i18next.js' in index.js will be more meaningful. (now its really easy to forget about this line)

Thanks a lot, you are doing a great job with i18next.

@jamuhl
Copy link
Member Author

jamuhl commented Nov 15, 2018

@vict-shevchenko that is right...the option to have multiple i18next instances gets dropped....but honestly that is a rather large edge case of using multiple instances -> in react-intl it's the only way to use multiple translation files - as i18next comes with namespaces the only use case having multiple instances (at least the only i see) is having differently formatted translation files.

What we might do is adding a get i18n from props here: https://github.com/i18next/react-i18next/blob/master/src/hooks/useTranslation.js#L26 or some other way to inject getting the i18n instance (eg. by using useContext) - so I18nextProvider and using react context API could be done outside of react-i18next for projects needing those multiple instances - what do you think?

@vict-shevchenko
Copy link

Hi, Jan!

Excuse me for not responding so long. Yet, had not changed to think through the approach. For sure, I am not aware of how often there is a scenario with multiple instances, and this is highly possible that people can solve their task in another way. (staying with one instance)

Regarding a solution for passing i18next instance - definitely keeping this logic out of react-i18next will do the job. 👍

I thought if we can somehow omit having own logic for storing an i18next instance and use React Context for it, as it was like designed for such purposes. But I really can not come up with a solution that keeps code as simple as it is now. :(

I also see that some library authors are getting rid of Provider components like here. So maybe we won't need I18nextProvider in future.

@DmacLoyaltyOne
Copy link

DmacLoyaltyOne commented Nov 20, 2018

I'm trying to get this to work using next.js and SSR
I'm running into the following problem:

In order to get this to work, I have to import
const { initReactI18n } = require('../../node_modules/react-i18next/dist/commonjs/hooks')
rather than
const { initReactI18n } = require('react-i18next/hooks')

Otherwise I get the following error:

\node_modules\react-i18next\hooks.js:1
(function (exports, require, module, __filename, __dirname) { export {

@jamuhl
Copy link
Member Author

jamuhl commented Nov 20, 2018

@DmacLoyaltyOne that's for now...when we release v10 we will set the entry points in package.json to the correct build. Currently react-i18next/hooks points to the es build which is optimized to be used in webpack and not on server.

@cthurston
Copy link

Love the hooks feature. Using it on two projects already.

Is it too early to be posting bugs? useTranslation(['ns1', 'ns2']) doesn't seem to merge in the second namespace.

@jamuhl
Copy link
Member Author

jamuhl commented Nov 30, 2018

@cthurston no not to early - quite the opposite - just go ahead and open those issues...just add somewhere that it's about the hooks version ;)

@jamuhl jamuhl mentioned this issue Dec 2, 2018
2 tasks
@gorkunov
Copy link

seems hooks works pretty well for me. Lack of SSR example with hooks was a problem :).

@jamuhl
Copy link
Member Author

jamuhl commented Dec 27, 2018

@gorkunov thank you for the feedback. we are aware of that and next.js sample is currently a open task: #591 (comment)

if time comes i'm sure @isaachinman will update next-i18next with it.

@isaachinman
Copy link
Contributor

Yes, I'll be updating next-i18next once Facebook and then react-i18next are updated first. It'll be awhile.

@jamuhl
Copy link
Member Author

jamuhl commented Dec 31, 2018

closing this - seems all currently needed is in the v10 "roadmap" issue

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

No branches or pull requests

8 participants