feat: refactor loadAndActivate #1609

vonovak · 2023-04-21T13:54:51Z

Description

this makes 2 changes to loadAndActivate: one changes the signature and the other is removing the notify param.

First, there's a reason why the existing activate call accepts locale and locales separately, because when you activate as in https://lingui.dev/ref/core#optionslocales

then the locale will be used for translating strings and locales will be used for number formatting here

js-lingui/packages/core/src/i18n.ts

Line 242 in 559a8c7

return number(this._locales || this._locale, format)(value)

maintaining the behavior from the example (locale="ar", locales="["en-UK", "ar-AS"]) would not be possible with how loadAndActivate is done currently, so I had to refactor it.

Additionally, I removed the notify parameter, whose description was: "This is useful for integration with frameworks as NextJS to avoid race-conditions during initialization."

However, the issues that were met when integrating with nextJS, as seen in lingui/swc-plugin#46 are just a result of not "gluing" nextJS / react with lingui correctly. The solution to the error is to change the gluing, not the i18n object.

Not emitting the change event is a workaround which, in fact, creates bugs as seen in lingui/swc-plugin@5fbcd0f

The React Provider relies on those events being emitted and giving the option to not emit is not going to help.

Thank you so much and sorry for maybe not sounding nice, I'm kinda busy, thank you for you review :)

more docs changes are needed but are out of scope of this PR. I plan another PR for the nextjs example to improve the "gluing".

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 · 2023-04-21T13:55:17Z

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	Apr 23, 2023 0:28am

github-actions · 2023-04-21T14:00:14Z

size-limit report 📦

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

codecov · 2023-04-21T14:06:54Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (67e20b4) 75.40% compared to head (208e1fe) 75.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1609      +/-   ##
==========================================
+ Coverage   75.40%   75.47%   +0.07%     
==========================================
  Files          76       77       +1     
  Lines        1980     1986       +6     
  Branches      518      518              
==========================================
+ Hits         1493     1499       +6     
+ Misses        375      374       -1     
- Partials      112      113       +1

Impacted Files	Coverage Δ
packages/core/src/i18n.ts	`64.51% <100.00%> (-2.65%)`	⬇️

... and 2 files with indirect coverage changes

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.

thekip · 2023-04-21T15:13:06Z

However, the issues that were met when integrating with nextJS, as seen in lingui/swc-plugin#46 are just a result of not "gluing" nextJS / react with lingui correctly. The solution to the error is to change the gluing, not the i18n object.

I agree that may be my solution is not the best. But moving gluing to the userland will cause problems with proper integration of lingui for the users and as consequences lower retention rate. What is really should be done, probably, changes to the provider, implementing kinda "distinctUntilChanged" strategy.

Instead of just deleting the functionality and leaving users hanging, maybe you could propose a better integration solution?

Also, once this PR gets merged, all the examples should be updated to reflect the changes, or maybe that could be done as part of the PR.

vonovak · 2023-04-21T17:53:20Z

hello 🙂
the way I see it, we have 3 options

have proper integration with frameworks like nextJS, maybe in a separate package (?). Note this may be good amount of work, eg. i18next has next-i18next package to accomplish that
have no integrations but have solid foundations that can be built upon, either in user land of by folks publishing their own packages
have broken integrations

Let's take it step by step. Right now, based on what I explained, we seem to have (3) and I'm aiming for (2).

Instead of just deleting the functionality and leaving users hanging, maybe you could propose a better integration solution?

please see changes to useLinguiInit() here. I'm not sure it's the best that can be done, but it works, and it's the best I can do on a friday evening 😄 .

I think the Provider is doing fine the way it is; it takes an i18n instance, passes it to consumers via context, re-renders them when the locale changes. It's pretty simple and that's good. But it's up for discussion later.

side question: I tested the changes locally.. the example projects have lingui installed in node_modules... maybe they could yarn link directly to the sources?

thanks

packages/core/src/i18n.ts

taozhou-glean · 2023-04-22T00:01:34Z

packages/core/src/i18n.ts

-    if (notify) {
-      this.emit("change")
-    }
+    this.emit("change")


just curious, what is the rationale here that always notify ? since previously, people may assume that change is only notified if locale changed, this might break that assumption

since previously, people may assume that change is only notified if locale changed

change was (and still is) previously emitted when calling activate or load, so this is in line with the current behavior

thekip · 2023-04-24T06:41:12Z

Let's take it step by step. Right now, based on what I explained, we seem to have (3) and I'm aiming for (2).

Sure, I understand your point.

i'm just thinking that this foundation could be made to support such integrations way easier. For example how @taozhou-glean is mentioned, don't fire a change event if there are no real change happened?

Something like:

loadAndActivate({ locale, locales, messages }: LoadAndActivateOptions) {
  this._locale = locale
  this._locales = locales || undefined

  this._messages[this._locale] = messages

  if ((JSON.stringify(this._messages[this._locale]) !== JSON.stringify(messages)) || this._locale !== locale) {
     this.emit("change")
  }
}

side question: I tested the changes locally.. the example projects have lingui installed in node_modules... maybe they could yarn link directly to the sources?

The contribution guide suggest using local npm repository with Verdaccio.

vonovak · 2023-04-24T07:06:10Z

For example how @taozhou-glean is mentioned, don't fire a change event if there are no real change happened?

The current situation is this: whenever load or activate is called, change event is emitted. If we start making the event emitter here "smarter" then that is not in line with how load+activate works currently. That's confusing because loadAndActivate, based on the name, should not behave differently.

Second, I don't think we need to be smarter here (and neither in the activate and load calls) - practically, how often will loadAndActivate be called? Probably (1) upon the application start and (2) when user changes locale somewhere in settings. I don't think we need to optimize the event emission for these cases; it very likely that when loadAndActivate is called then something is going to change - otherwise the user wouldn't call it in the first place.

Third, the JSON.stringify comparison: That's going to be slow for big message catalogs, and 99.9% of time the change event will end up being emitted, and the time spent stringifying will be a waste of resources. Also, this time-consuming comparison is on the hot path of application start-up.

thekip · 2023-04-24T08:51:41Z

If naming is an issue - let's rename it.

I don't think we need to optimize the event emission for these cases;

It's not about performance, but about easiness of integration. For the simplicity, it even could be just reference comparison, because in most cases that would be enough.

  if ((this._messages[this._locale] !== messages) || this._locale !== locale) {
     this.emit("change")
  }

vonovak · 2023-04-24T09:22:36Z

It's not about performance, but about easiness of integration.

Well, it's not about performance, but only while the perf is good.

sorry, but I just don't see the issue with the event emitting here. What is the integration problem? load and activate work in a certain way since a long time ago. This new function just follows their behavior.

For simplicity, it even could be just reference comparison

yeah but then when people take a message catalog, mutate it by adding a few strings in it, its reference will be the same, and event will not be emitted when it should.

thekip · 2023-04-24T09:33:46Z

The issue is, it's quite hard to come up with this:

 const isClient = typeof window !== "undefined"

  if (!isClient && locale !== i18n.locale) {
    // there is single instance of i18n on the server
    // note: on the server, we could have an instance of i18n per supported locale
    // to avoid calling loadAndActivate for (worst case) each request, but right now that's what we do
    i18n.loadAndActivate({ locale, messages })
  }
  if (isClient && i18n.locale === undefined) {
    // first client render
    i18n.loadAndActivate({ locale, messages })
  }

  useEffect(() => {
    const localeDidChange = locale !== i18n.locale
    if (localeDidChange) {
      i18n.loadAndActivate({ locale, messages })
    }
  }, [locale])

for the regular developer. That issue I tried to tackle. Make integration more straightforward.

yeah but then when people take a message catalog, mutate it by adding a few strings in it, its reference will be the same, and event will not be emitted when it should.

Anyway, this wouldn't work because messages should be compiled before. It's error-prone approach, because some messages added on the fly would work, some not. It's even better if it fails first.

Let's finish this discussion, i agree with you, let's leave it as you wrote that.

andrii-bodnar · 2023-04-25T12:46:41Z

@vonovak @thekip @taozhou-glean let's merge this?

bravo-kernel · 2023-04-26T14:35:15Z

examples/nextjs-babel/src/i18n.ts

+    // there is single instance of i18n on the server
+    // note: on the server, we could have an instance of i18n per supported locale
+    // to avoid calling loadAndActivate for (worst case) each request, but right now that's what we do
+    i18n.loadAndActivate({ locale, messages })


Arguments should not be an object here correct?

hi, they should be an object:

js-lingui/packages/core/src/i18n.ts

Line 187 in 9989963

loadAndActivate({ locale, locales, messages }: LoadAndActivateOptions) {

Yes, I just rechecked, apologies. Nice job 💯

refactor: remove notify param

ebce24f

vercel bot deployed to Preview April 21, 2023 13:57 View deployment

refactor: loadAndActivate

34bd46a

vonovak force-pushed the feat/remove-notify-param branch from 9a20fde to 34bd46a Compare April 21, 2023 14:01

vercel bot deployed to Preview April 21, 2023 14:04 View deployment

vonovak marked this pull request as ready for review April 21, 2023 14:18

vonovak mentioned this pull request Apr 21, 2023

chore: official v4 release #1608

Merged

docs: fix wording

32d2f1c

vercel bot deployed to Preview April 21, 2023 14:39 View deployment

andrii-bodnar added this to the v4 milestone Apr 21, 2023

refactor: update existing useLinguiInit calls

d6dedf2

vercel bot deployed to Preview April 21, 2023 17:38 View deployment

taozhou-glean reviewed Apr 22, 2023

View reviewed changes

packages/core/src/i18n.ts Outdated Show resolved Hide resolved

taozhou-glean reviewed Apr 22, 2023

View reviewed changes

docs: update loadAndActivate TSdoc

208e1fe

vercel bot deployed to Preview April 23, 2023 12:28 View deployment

thekip approved these changes Apr 25, 2023

View reviewed changes

taozhou-glean approved these changes Apr 25, 2023

View reviewed changes

andrii-bodnar approved these changes Apr 26, 2023

View reviewed changes

andrii-bodnar merged commit 7e04168 into lingui:next Apr 26, 2023
14 checks passed

bravo-kernel reviewed Apr 26, 2023

View reviewed changes

vonovak deleted the feat/remove-notify-param branch April 26, 2023 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor loadAndActivate #1609

feat: refactor loadAndActivate #1609

vonovak commented Apr 21, 2023 •

edited

Loading

vercel bot commented Apr 21, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

codecov bot commented Apr 21, 2023 •

edited

Loading

thekip commented Apr 21, 2023

vonovak commented Apr 21, 2023 •

edited

Loading

taozhou-glean Apr 22, 2023

vonovak Apr 23, 2023

thekip commented Apr 24, 2023

vonovak commented Apr 24, 2023 •

edited

Loading

thekip commented Apr 24, 2023

vonovak commented Apr 24, 2023

thekip commented Apr 24, 2023

andrii-bodnar commented Apr 25, 2023

bravo-kernel Apr 26, 2023

vonovak Apr 26, 2023 •

edited

Loading

bravo-kernel Apr 26, 2023

feat: refactor loadAndActivate #1609

feat: refactor loadAndActivate #1609

Conversation

vonovak commented Apr 21, 2023 • edited Loading

Description

Types of changes

Checklist

vercel bot commented Apr 21, 2023 • edited Loading

github-actions bot commented Apr 21, 2023 • edited Loading

size-limit report 📦

codecov bot commented Apr 21, 2023 • edited Loading

Codecov Report

thekip commented Apr 21, 2023

vonovak commented Apr 21, 2023 • edited Loading

taozhou-glean Apr 22, 2023

Choose a reason for hiding this comment

vonovak Apr 23, 2023

Choose a reason for hiding this comment

thekip commented Apr 24, 2023

vonovak commented Apr 24, 2023 • edited Loading

thekip commented Apr 24, 2023

vonovak commented Apr 24, 2023

thekip commented Apr 24, 2023

andrii-bodnar commented Apr 25, 2023

bravo-kernel Apr 26, 2023

Choose a reason for hiding this comment

vonovak Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

bravo-kernel Apr 26, 2023

Choose a reason for hiding this comment

vonovak commented Apr 21, 2023 •

edited

Loading

vercel bot commented Apr 21, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

codecov bot commented Apr 21, 2023 •

edited

Loading

vonovak commented Apr 21, 2023 •

edited

Loading

vonovak commented Apr 24, 2023 •

edited

Loading

vonovak Apr 26, 2023 •

edited

Loading