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

feat: memoizable react translations #1721

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

vonovak
Copy link
Collaborator

@vonovak vonovak commented Jul 3, 2023

Description

this is an improvement for #1690 (#1690 (comment)), addresses difficulty with the current api: https://lingui.dev/tutorials/react-patterns#memoization-pitfall

I'm opening this for feedback; after potential issues are addresses, I can write docs.

This allows the following construct to work correctly:

import { msg } from "@lingui/macro";

export const WelcomeMsg = () => {
  const { _ } = useLingui();

  const memoizedWelcome = useMemo(() => {
    return _(msg`Welcome`);
  }, [_]);

  return <Text>greeting: {memoizedWelcome}</Text>;
}

It feels like there are a lot of ways to achieve more or less the same thing now, with t macro, Trans, msg... I think it's okay to add this, if there are quality docs.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Fixes # (issue)

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Jul 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2023 9:05am

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.76 KB (0%)
./packages/detect-locale/dist/index.mjs 721 B (0%)
./packages/react/dist/index.mjs 1.59 KB (0%)
./packages/remote-loader/dist/index.mjs 7.24 KB (0%)

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (43486dc) 75.87% compared to head (4369daf) 75.87%.

❗ Current head 4369daf differs from pull request most recent head fee7f29. Consider uploading reports for the commit fee7f29 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1721   +/-   ##
=======================================
  Coverage   75.87%   75.87%           
=======================================
  Files          80       80           
  Lines        2052     2052           
  Branches      526      526           
=======================================
  Hits         1557     1557           
  Misses        382      382           
  Partials      113      113           
Impacted Files Coverage Δ
packages/react/src/I18nProvider.tsx 91.30% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link

@niksauer niksauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly what we need ❤️

@timofei-iatsenko
Copy link
Collaborator

By the way, i also thought about making something like that as default solution in docs instead of using global t macro which is known as error-prone. I thought to make it more explicit:

import { t } from "@lingui/macro";

// ❌ Bad because there no direct relation between `t` and `useLingui`
export const WelcomeMsg = React.memo(() => {
  useLingui();
  return <Text>greeting: {t`Welcome`}</Text>;
});
import { t } from "@lingui/macro";

// ❌ Better than first one, but still bad because too verbose, 
//  and Developers can omit `(i18n)` part and don't get consequences immediately
export const WelcomeMsg = React.memo(() => {
  const { i18n } = useLingui();
  return <Text>greeting: {t(i18n)`Welcome`}</Text>;
});

So I thought about implementing a new macro useLingui:

import { useLingui } from "@lingui/macro";

// ✅ Good, as you see direct dependecy on the hook, 
//    but really hard to implement from Transpilation point of view
export const WelcomeMsg = React.memo(() => {
  const { t } = useLingui();
  return <Text>greeting: {t`Welcome`}</Text>;
});

But thought about that once again and i also came to similar to yours solution, that instead of creating new complicated macro we can achieve the same result with existing code. We only need to change documentation to encourage people to use different methods:

import { useLingui } from "@lingui/macro";

export const WelcomeMsg = React.memo(() => {
  const { i18n } = useLingui();

  // expand macro and mark message for extraction with `msg`
  return <Text>greeting: {i18n.t(msg`Welcome`)}</Text>;
});

Despite the fact, we came to here solving diffrent issues, we end up with a similar solution.
As a further steps i think we need to replace all usages and mentions of global t macro to _(msg) in React context in docs.

@vonovak
Copy link
Collaborator Author

vonovak commented Jul 4, 2023

As a further steps i think we need to replace all usages and mentions of global t macro to _(msg) in React context in docs.

@thekip so just to make sure I understand :)
you'd use the code from this PR and recommend write code as in these examples?

(I changed the examples to use <Button title={...} /> because for the previous examples, using <Trans /> would be better :) )

import { msg } from "@lingui/macro";

export const WelcomeMsg = () => {
  const { _ } = useLingui();

  return <Button title={_(msg`Do something`)} />
}

or in case someone needs to use memo:

import { msg } from "@lingui/macro";

export const WelcomeMsg = () => {
  const { _ } = useLingui();

  const memoizedTitle = useMemo(() => {
    return _(msg`Do something`);
  }, [_]);

  return <Button title={memoizedTitle} />
}

I really like this one that you suggested, thought about it as well, but I don't think I have the bandwidth to explore the transpilation :/

import { useLingui } from "@lingui/macro"; // or "@lingui/react"?

// ✅ Good, as you see direct dependency on the hook, 
//  but really hard to implement from Transpilation point of view
export const WelcomeMsg = () => {
  const { t } = useLingui();
  return <Button title={t`Do something`} />
}

which would transpile to this?

import { useLingui } from "@lingui/react";

export const WelcomeMsg = () => {
  const { t } = useLingui();
  return (
    <Button
      title={t(
        /*i18n*/ {
          id: "some_hash",
          message: "Do something",
        },
      )}
    />
  );
};

maybe we can keep the docs as they are and point out to this possible improvement, maybe someone will contribute it 😄

@timofei-iatsenko
Copy link
Collaborator

you'd use the code from this PR and recommend write code as in these examples?

@vonovak yes, exactly.

Regarding new macro:

import { useLingui } from "@lingui/macro"; // or "@lingui/react"?

export const WelcomeMsg = () => {
  const { t } = useLingui();
  return <Button title={t`Do something`} />
}

This ideally should be transpiled to something like

import { useLingui } from "@lingui/macro"; // or "@lingui/react"?

export const WelcomeMsg = () => {
  const { i18n } = useLingui();
  return <Button title={i18n._(`<expanded macro>`)} />
}

So to achieve this we need to find all usages of t than traverse scope upper and somehow identify boundaries of react component, then find in the react component useLingui() call and rewrite it. or it could be done vice versa, from hook down to the usages. There could be so many edge cases and still problems with memo because you could not refer to t as a dependency in runtime. Also, someone may want to pass t function as parameter somewhere expecting this to work.

So i prefer may be less elegant but more future-proof solution.

@vonovak
Copy link
Collaborator Author

vonovak commented Jul 5, 2023

This ideally should be transpiled to something like

I believe the transpilation result you gave would be more complicated than needed because it transforms const { t } = useLingui(); to const { i18n } = useLingui();, and if there is a useMemo such as in the following example, we'd need to transform hook dependencies as well...

import { useLingui } from "@lingui/macro"; // or "@lingui/react"?

export const WelcomeMsg = () => {
  const { t } = useLingui();

  const greeting = useMemo(() => {
    return t`hello world`
  }, [t]);

  return <Button title={greeting} />
}

Anyhow, let me wrap this up, I'll add the docs for what I added here and then mark as ready for review :)

@vonovak vonovak marked this pull request as ready for review July 5, 2023 09:04
@vonovak
Copy link
Collaborator Author

vonovak commented Jul 11, 2023

@andrii-bodnar can you please review / merge? I have some other stuff in the pipeline and would like to get this merged first, thanks :)

edit: I see you're on vacation, hope you enjoyed it! :)

Copy link
Contributor

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vonovak thank you!

Sorry for the late response 🙏

@andrii-bodnar andrii-bodnar merged commit 96e7def into lingui:main Jul 18, 2023
@vonovak vonovak deleted the feat/memoizable-translations branch August 10, 2023 10:19
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 this pull request may close these issues.

4 participants