feat: add js/node core translation api for usage without macros

[j4hr3n]

A suggested change to add an additional core API method. This method can be used without including macros. It accepts a message descriptor and the babel message extraction plugin has been updated to handle extraction of this syntax.

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

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]

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

Name	Status	Preview	Comments	Updated
js-lingui	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 31, 2023 at 6:34AM (UTC)

[github-actions]

size-limit report 📦

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

[codecov]

Codecov Report

Patch coverage: 94.11% and project coverage change: +0.32 🎉

Comparison is base (f2961fc) 75.41% compared to head (37ef4b8) 75.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1564      +/-   ##
==========================================
+ Coverage   75.41%   75.74%   +0.32%     
==========================================
  Files          77       77              
  Lines        1973     1987      +14     
  Branches      517      522       +5     
==========================================
+ Hits         1488     1505      +17     
+ Misses        374      371       -3     
  Partials      111      111              

Impacted Files	Coverage Δ
...ackages/babel-plugin-extract-messages/src/index.ts	93.87% <93.75%> (-0.25%)	⬇️
packages/core/src/i18n.ts	73.13% <100.00%> (+6.46%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[timofei-iatsenko]

Why need to create another api surface for quite rare case if you can directly write

/*i18n*/ {id: "...", message: "..."}

The problem with this exact implemetation - it fixes only your case, and you assume everyone would use it like that.

The code doesn't check that i18n is imported from lingui/core, the code doesn't check that i18n could be renamed, for example

import {i18n as lingui} from "@lingui/core"

lingui.t({id: "...", message: "..."}) // <-- Oops!

There already a way to mark your messages for translation with /*i18n*/ comment.

[j4hr3n]

Why need to create another api surface for quite rare case if you can directly write

/*i18n*/ {id: "...", message: "..."}

The problem with this exact implemetation - it fixes only your case, and you assume everyone would use it like that.

The code doesn't check that i18n is imported from lingui/core, the code doesn't check that i18n could be renamed, for example

import {i18n as lingui} from "@lingui/core"

lingui.t({id: "...", message: "..."}) // <-- Oops!

There already a way to mark your messages for translation with /*i18n*/ comment.

Thanks for checking out my PR @thekip!

I can adjust my PR to check for proper import and aliasing. The whole point of the PR is to allow users who don't want macros to avoid having to use i18n._ and decorating with the /*i18n*/ comment if they want to use descriptors. My goal here is to just slightly improve the DX for non-macro users who need strings with comments, message and custom IDs.

[andrii-bodnar]

@j4hr3n thank you for the contribution!

I can remember a couple of recent similar cases mentioned somewhere in the Discord chat or here in the comments

It might be a common issue for such cases when the macro is unacceptable

[j4hr3n]

@thekip I've added support for aliasing i18n now

[timofei-iatsenko]

Well, if we decided to go that way, may be it's better to improve extraction from i18n._ instead of creating an alias which is actually not an alias because works differently? i'm expecting confusing people on that aspect.

I'm not against creating alias for more aligned DX with macro, but with this PR we would have 2 functions which serve the same purpose but behave slightly differently.

[j4hr3n]

Well, if we decided to go that way, may be it's better to improve extraction from i18n._ instead of creating an alias which is actually not an alias because works differently? i'm expecting confusing people on that aspect.

I'm not against creating alias for more aligned DX with macro, but with this PR we would have 2 functions which serve the same purpose but behave slightly differently.

@thekip

I can agree that it might not be the most optimal solution to the problem. That being said Lingui is advocated as a universal, use it everywhere, javascript library. If you have to use i18n._ when you can't/won't use macros it feels like that part of the ecosystem is sort of left out. It works, but the DX is not that great.

[timofei-iatsenko]

also withNodeMessageDescriptor, withAliasedNodeMessageDescriptor etc, there shouldn't be node in the name

[timofei-iatsenko]

@andrii-bodnar could you help @j4hr3n here to write more descriptive doc for the method? I think it should be stated from the doc that the main difference here is how this method is treated by extractor. (not the signature of the method, how some may understand from this doc)

And probably we also could state that if someone wants to use lingui without macro it's better to start from this method.

[andrii-bodnar]

I'm going to release it within the v4.0.0-next.5 and then check the possibility of docs improvement in a separate PR

[timofei-iatsenko]

I just realized that there is a use case which is most likely would be a default on the server side:

 const i18n = setupI18n({})

// here extractor would not work
i18n.t(...)

On the server side usually everything is linked to the current request to avoid leaking data from one user to another.

// pseudocode

const app = express()

 // init i18n middleware
app.use((req, res) => {
  const locale = detectLocale(req);
  req.i18n = setupI18n({locale});
  next()
})

app.get('/', (req, res) => {
  res.send(req.i18n.t('hello world'))
})

So this would not work. The only one, reliable way to extract in this case it is manual annotation what should be extracted and what should not.

The same would be for client side usage without macro:

function MyComponent(props) {
  const {i18n} = useLingui();

  <title>{i18n.t(...)}</title>
}

So i'm still not sure that this solution is really needed for a lib where is already reliable way to do that exists.

[j4hr3n]

I just realized that there is a use case which is most likely would be a default on the server side:

 const i18n = setupI18n({})

// here extractor would not work
i18n.t(...)

On the server side usually everything is linked to the current request to avoid leaking data from one user to another.

// pseudocode

const app = express()

 // init i18n middleware
app.use((req, res) => {
  const locale = detectLocale(req);
  req.i18n = setupI18n({locale});
  next()
})

app.get('/', (req, res) => {
  res.send(req.i18n.t('hello world'))
})

So this would not work. The only one, reliable way to extract in this case it is manual annotation what should be extracted and what should not.

The same would be for client side usage without macro:

function MyComponent(props) {
  const {i18n} = useLingui();

  <title>{i18n.t(...)}</title>
}

So i'm still not sure that this solution is really needed for a lib where is already reliable way to do that exists.

@thekip

Even with that usecase in mind I feel like non-macro users deserve a slightly better DX than doing i18n._. Even though there's a supported way of extracting those messages today doesn't mean its a great use case outside of macros.

With this new core API marked as experimental I feel like we could incrementally add support to cases like this. I'd be happy to do so.

[timofei-iatsenko]

You are opened are pandora's box. Good luck

[timofei-iatsenko]

Let me rephrase and summarize the problem which you want to solve with this pr.

Make better DX for non-macro users.

Let's dig deeper into this statement. What exactly wrong with current DX?

I think we can highlight pain points:

1. You need to write /*i18n*/ comment to mark messages for extraction
2. current i18n._ method is working for extraction, but not all forms supported.
3. You don't like the _ method naming.

Let's start from the history. Originally, extractor did not extract from i18n._ at all. Annotation was always needed.
Then one of the contributors added support for extraction from i18n._. That was a first attempt to "improve" DX for non-macro users. On that moment, project was not actively maintained, and current maintainers didn't spend too much time to check this PR and think of all possible consequences and just merged it.

But that implementation had a lot of problems which we're still fixing! It works not consistently. It matches just i18n._ pattern, so it could clash with another user-land functions. It doesn't support all forms (like message descriptor. It has ambiguity with other forms produced by macro so in some places I was forced to relax a bit some checks to make it works and avoid issues.

It was a mistake, that should never be merged into sourceode in the form it was presented. This gives people false expectations about how this should work.

The second attempt, happened in this PR. Made on wrong assumptions from that first i18n._ improvement. With my "help" author added checking for the imports which in turn broke all other (very popular) usecases.

There are simply no way to make it work in the form as it written right now. The static analysis is just not powerful enough to understand all usages which should be extracted. We would always have some trade-offs in extraction.

Let's come back to the "root" of the issue.

I think manual annotation is the root. They are hard to write by hand and easy to forget.

What we can do about it? Could we look to a problem from the different perspective? If we don't like how it's made now, maybe annotation itself could be changed instead of trying to support all possible usage cases?

I see few options which would "just work", thanks to typescript, and will lack all this issues. Annotations could be part of the code, part of the type system.

type AnnotatedMessage = {
  //...
}

i18n.t(message: AnnotatedMessage) {
 // ...
}

function msg(string, placeholder): AnnotatedMessage {
 // ...
}

// =======

i18n.t(`Hello`) // < -- typescript error. string is not assignable to AnnotatedMessage
i18n.t(msg`Hello`) // <-- no error, developer now will not forget to annotate!

Extractor should be taught to search msg calls from @lingui/core not the i18n.t because later could come from different places in different forms. msg itself is just a helper function which does nothing, just for type-safety and marking string for extraction instead of /*i18n*/ comment.

I don't like msg name itself because it's ambiguous with the macro, but i didn't come up with better.

Does it make sense?

[timofei-iatsenko]

@j4hr3n @andrii-bodnar fyi