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

@tolgee/react useTranslate hook not returning new t function on translation change #3247

Closed
avainola opened this issue Sep 11, 2023 · 1 comment · Fixed by #3248
Closed

Comments

@avainola
Copy link

Reporting with the assumption (based on https://tolgee.io/js-sdk/integrations/react/api#function-t) that t function from useTranslate hook should trigger component update/re-render when translations change.

After a bit of debugging (but without having deep understanding of the inner workings of @tolgee/core), I think there is a race condition happening that sometimes results in new t function not being created after translations change. Since it is a race condition, it is not easy to create a minimal reproducible example, but if you are not able to reproduce it, I will try to find time to do it. I'll try to share/explain my finding and if my assumptions are wrong or I have misunderstood how it should be working, then sorry for the confusion.

The problem seems to be in the useTranslateInternal hook, more specifically these LOCs.
Problem:

  1. https://github.com/tolgee/tolgee-js/blob/main/packages/react/src/useTranslateInternal.ts#L40
const isLoaded = tolgee.isLoaded(namespaces);

isLoaded is hook scope variable, beings re-evaluated every time the component using the useTranslate hook is re-rendering. This mean it does not always reflect to actual tolgee instance state.
I think it would be more correct to push this value from the event stream when it changes instead of relying on component re-render to have correct/up-to-date value.

  1. https://github.com/tolgee/tolgee-js/blob/main/packages/react/src/useTranslateInternal.ts#L42-L55
useEffect(() => {
   const subscription = tolgee.onNsUpdate(rerender);
   subscriptionRef.current = subscription;
   if (!isLoaded) {
      subscription.subscribeNs(namespaces);
   }
   subscriptionQueue.current.forEach((ns) => {
      subscription!.subscribeNs(ns);
   });

   return () => {
      subscription.unsubscribe();
   };
}, [isLoaded, namespacesJoined, tolgee]);

This effect creates new subscription and unsubscribes from the (previous) subscription that will be replaced by the new. The culprit (and the question I haven't figured out - "why is it needed?") here is that it depends on isLoaded (which depends on component re-renders), meaning unsubscribing and creating a new subscription is not determined only by input arguments and tolgee state, but also on how often the component is re-rendering.

  1. https://github.com/tolgee/tolgee-js/blob/main/packages/core/src/Controller/Events/EventEmitterSelective.ts#L77-L86
emit(ns?: string[], delayed?: boolean) {
   if (isActive()) {
      queue.push(ns);
      if (!delayed) {
          solveQueue();
      } else {
          setTimeout(solveQueue, 0);
      }
   }
},

https://github.com/tolgee/tolgee-js/blob/main/packages/core/src/Controller/Events/Events.ts#L58-L60

self.onCacheChange.listen(({ value }) =>
   self.onUpdate.emit([value.namespace], true)
);

Translation changes are emitted asynchronously (with delayed: true).

Now, If we put all this together:

  • It is possible that the component re-renders exactly when tolgee cache has updated, but the subscribers are yet not notified about the update.
  • If during this render isLoaded changes from false to true, it will unsubscribe, but not re-subscribe, since the useEffect hook is subscribing conditionally. This would not be a problem if event emitter would consider the subscribers at the moment of the event happening, but since it is not, it does not call the subscribers that unsubscribe after the event happening, but before the callbacks/handlers are being called.
  • Since the race condition is triggered by component re-render, the issue is only revealed when the component is actually relying on the t function causing a re-render (calling t function inside React.useMemo that depends on t).

Unfortunately, I don't know the library well enough to suggest a change/PR at this point.

@stepan662
Copy link
Collaborator

Hey, thanks for detailed report. I think the mentioned part can be omited (if (!isLoaded) {).
It's already quite some time I've written this code and I think that was supposed to be some optimization, but I guess it did more harm than good.

stepan662 added a commit that referenced this issue Sep 11, 2023
Fixing #3247 and during that
I've found another bug, which occurs when in strict mode when not using
suspense, that loading state is resolved too early in TolgeeProvider
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

Successfully merging a pull request may close this issue.

2 participants