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(macro): add useLingui macro #1859

Merged
merged 14 commits into from
Mar 1, 2024

Conversation

thekip
Copy link
Collaborator

@thekip thekip commented Feb 22, 2024

#1852

Adds new macro useLingui which is drastically simplify working with non-jsx messages in react components:

import { useLingui } from '@lingui/macro';
function MyComponent() {
  const { t } = useLingui();
  const a = t`Text`;
}

becomes:

import { useLingui } from "@lingui/react";
function MyComponent() {
  const { _: _t } = useLingui();
  const a = _t(
    /*i18n*/
    {
      id: "xeiujy",
      message: "Text",
    }
  );
}`

All syntax flavors of original t function is also supported.

Also, it's possible to use returned t as dependency in react hooks:

import { useLingui } from '@lingui/macro';
function MyComponent() {
  const { t } = useLingui();
  const a = useMemo(() => t`Text`, [t]);
}

// becomes:
import { useLingui } from "@lingui/react";
function MyComponent() {
  const { _: _t } = useLingui();
  const a = useMemo(
    () =>
      _t(
        /*i18n*/
        {
          id: "xeiujy",
          message: "Text",
        }
      ),
    [_t]
  );
}

The implementation still has some not covered cases, but I believe it should cover 95% regular cases and it's already good enough to give it a try.

Description

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
  • Examples 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 the necessary documentation (if appropriate)

Copy link

vercel bot commented Feb 22, 2024

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 Mar 1, 2024 8:49am

Copy link

github-actions bot commented Feb 22, 2024

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.86 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

packages/macro/src/macroJs.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 89.55224% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 77.04%. Comparing base (107bd02) to head (ffd3a64).
Report is 2 commits behind head on main.

Files Patch % Lines
packages/macro/src/macroJs.ts 82.60% 2 Missing and 2 partials ⚠️
packages/macro/src/index.ts 91.89% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1859      +/-   ##
==========================================
+ Coverage   76.66%   77.04%   +0.37%     
==========================================
  Files          81       82       +1     
  Lines        2083     2139      +56     
  Branches      532      548      +16     
==========================================
+ Hits         1597     1648      +51     
- Misses        375      378       +3     
- Partials      111      113       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dan-dr
Copy link
Contributor

dan-dr commented Feb 24, 2024

Seems to have the same issue I had when extracting:
{t`What ${plural(3, { zero: 'yes', other: 'no' })} Up`}
is wrongly extracted as
image

maybe it's a problem with the extractor, but i'm pretty sure its a more a tokenizer/macro issue.
Also, there's (maybe rare) cases where you want the i18n from the useLingui, which currently isn't supported. i.e.:

const { i18n, _ } = useLingui();
const { t } = useLinguiMacro();
const [locale, setLocale] = React.useState(i18n.locale);

@dan-dr
Copy link
Contributor

dan-dr commented Feb 24, 2024

thinking about it, I think a better way may be when the references are processed in the index file, add the t nodes thatt are from useLingui to the jsNodes array.

@dan-dr
Copy link
Contributor

dan-dr commented Feb 25, 2024

refactored your PR per my last comment:
https://github.com/dan-dr/js-lingui/tree/feature/use-lingui-macro

In my implementation:

  • I prefer that the macro imitates the standard useLingui but replaces the _ function. so if you need i18n you can use just the macro, instead of resulting code having duplicate useLingui hooks (and an extra render?)
  • pre-process useLingui in index file
    • set needsUseLinguiImport to true
    • report incorrect macro syntax
    • add all references of destructured t function to jsNodes
      • this eliminates any recursive macro issues
      • also filters out array expression to support dependency array references
    • add processing of these references to macroJs (similar to other macros)
    • file-scope rename the t destruction to a unique identifier and add to nameMap to support renaming when destructing
      • required. { t: trans } = useLingui() is similar to import { useLingui as useLinguiMacro } from useLingui

@thekip thekip marked this pull request as draft February 26, 2024 08:42
@thekip
Copy link
Collaborator Author

thekip commented Feb 26, 2024

@dan-dr i cherry-picked your changes to the branch of this PR.

I'm going to re-check your changes, add more tests, and mark PR as ready when I finish. Thanks for help and contributing.

@thekip
Copy link
Collaborator Author

thekip commented Feb 26, 2024

  1. Added usLingui to LinguiConfig.runtimeConfigModules to be consistent with other macro
  2. Added tests for validation errors
  3. Fixed typings and add typetest for i18n usage from useLingui
  4. added extractor test cases for useLingui

There still issue if the renamed import of useLingui is existing in the file and has renamed specifier, but the same error would be with any other macro, and i dont rembmer any user have issue with that.

import { useLingui as useLinguiMacro } from '@lingui/macro';
import { useLingui as useLinguiRenamed  } from '@lingui/react';

function MyComponent() {
  const { _ } = useLinguiRenamed();

  console.log(_);
  const { t } = useLinguiMacro();
  const a = t`Text`;
}

I'm going to fix it in next iterations in another PR's. I'm already working on refactoring which will help with experimental extractor and fix edge cases with renamed imports as well.

@thekip
Copy link
Collaborator Author

thekip commented Feb 26, 2024

I updated documentation for useLingui macro. I feel that documentation could be better, but i did my best honestly.

@thekip thekip marked this pull request as ready for review February 26, 2024 12:39
@thekip thekip mentioned this pull request Feb 28, 2024
8 tasks
@thekip
Copy link
Collaborator Author

thekip commented Feb 29, 2024

@andrii-bodnar when it can be merged? I'm thinking of merging it to the main and creating a pre-release (with -next or -beta suffix) Want to tests it thoroughly in the ecosystem first.

@andrii-bodnar
Copy link
Contributor

andrii-bodnar commented Feb 29, 2024

@thekip I can release it with the next tag. The release process already has this option

@thekip
Copy link
Collaborator Author

thekip commented Feb 29, 2024

So let's merge it, so i can concentrate on this #1867

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.

@thekip @dan-dr @vonovak thanks for your efforts and contributions!

Overall it looks good to me, but I was concerned about the confusion it might cause for developers trying to understand the difference between the useLingui macro and the useLingui React hook. Even now, it's a bit confusing at the documentation level.

The use<something> is a typical naming convention for React hooks. In our case, the useLingui macro might be confusing because of this naming convention for hooks.

Perhaps we need to better document the difference or consider choosing a different name for the macro.

What do you think guys?

website/docs/tutorials/react-native.md Show resolved Hide resolved
website/docs/tutorials/react-native.md Show resolved Hide resolved
website/docs/tutorials/react.md Outdated Show resolved Hide resolved
website/docs/tutorials/react.md Show resolved Hide resolved
website/docs/tutorials/react.md Outdated Show resolved Hide resolved
website/docs/tutorials/react.md Outdated Show resolved Hide resolved
Co-authored-by: Andrii Bodnar <andrii.bodnar@crowdin.com>
@thekip
Copy link
Collaborator Author

thekip commented Feb 29, 2024

The use<something> is a typical naming convention for React hooks. In our case, the useLingui macro might be confusing because of this naming convention for hooks.

Perhaps we need to better document the difference or consider choosing a different name for the macro.

useLingui macro is a hook as well. Could only be used in the context where runtime's useLingui used. So use<something> is a valid name for it.

This similar to Trans, which has two versions, macro and runtime.

I think naming is fine, but documentation could be improved.

@thekip
Copy link
Collaborator Author

thekip commented Feb 29, 2024

Guys, i believe we can improve documentation any time later, that should not block merging the feature.

@andrii-bodnar
Copy link
Contributor

andrii-bodnar commented Mar 1, 2024

@thekip thanks for the clarification! Could you please address the 3 remaining discussions?

#1859 (comment)
#1859 (comment)
#1859 (comment)

Then, I am going to merge this to the next branch and release 4.8.0 with the next tag.

@thekip
Copy link
Collaborator Author

thekip commented Mar 1, 2024

@andrii-bodnar @vonovak regarding naming, i think we can rise the discussion. I believe the initial idea for macro was a "subset" of the original params from runtime versions (as original author said). So they signatures originally had to match to each other.

But since then react/js ecosystem shifted towards typescript and quality typings and high type safety become a more valuable than ever. That resulted in changes in lingui, where runtime and macro version got they own typings with different signatures to highlight for developers different usages of macro/runtime versions.

So now we have Trans from /react and /Trans from macro which are diffrent components. This PR will add useLingui from /macro in additional to /react.
This is indeed confusing. It's also confusing for IDEs, they can automatically add import to incorrect symbol and user may spend hours to understand why it doesn't work as expected.

I think, maybe it's time to change the naming to something more explicit?

Trans / TransM and useLingui / useLinguiM

The Plural Select and SelectOrdinal doesn't have runtime counterparts, they are traspiled to Trans, but i believe they should be renamed to follow the same naming convention:

PluralM SelectM and SelectOrdinalM

What do you think?

@thekip
Copy link
Collaborator Author

thekip commented Mar 1, 2024

@andrii-bodnar

Then, I am going to merge this to the next branch and release 4.8.0 with the next tag.

Could next release be created from "main" branch? I don't see a reason for branching here

@thekip
Copy link
Collaborator Author

thekip commented Mar 1, 2024

@andrii-bodnar

@thekip thanks for the clarification! Could you please address the 3 remaining discussions?

#1859 (comment)
#1859 (comment)
#1859 (comment)

Reverted the docs, and just resolved rest, there are nothing to do about them.

@andrii-bodnar
Copy link
Contributor

I think, maybe it's time to change the naming to something more explicit?

Sounds interesting. Let's move this to a separate discussion and consider it in planning the v5 release. Currently, I don't have other ideas, but the M suffix looks a bit weird to me.

Could next release be created from "main" branch? I don't see a reason for branching here

When merging into the main branch, the documentation will be automatically deployed to lingui.dev. Also, if I remember correctly, the next release should be triggered from the next branch according to the Lerna configuration.

@andrii-bodnar andrii-bodnar changed the base branch from main to next March 1, 2024 12:42
@andrii-bodnar andrii-bodnar merged commit 17f84e8 into lingui:next Mar 1, 2024
18 checks passed
@thekip thekip deleted the feature/use-lingui-macro branch March 1, 2024 14:02
@andrii-bodnar
Copy link
Contributor

I was going to release it today but GitHub is currently experiencing an outage with Actions 😬

Probably better to do it on Monday.

@andrii-bodnar
Copy link
Contributor

andrii-bodnar pushed a commit that referenced this pull request Mar 6, 2024
Co-authored-by: ddyo <danrosenshain@gmail.com>
@thekip thekip mentioned this pull request Mar 11, 2024
3 tasks
andrii-bodnar pushed a commit that referenced this pull request Mar 18, 2024
Co-authored-by: ddyo <danrosenshain@gmail.com>
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.

Improve React syntax with useLingui
4 participants