tolgee / tolgee-js

Tolgee JavaScript libraries monorepo
https://tolgee.io
MIT License
219 stars 24 forks source link

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

Closed avainola closed 9 months ago

avainola commented 9 months ago

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.

  2. 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.

  3. 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:

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

stepan662 commented 9 months ago

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.