create-ignite-app icon indicating copy to clipboard operation
create-ignite-app copied to clipboard

Replace 'es2015-i18n-tag' w/ 'lingui'

Open adamsoderstrom opened this issue 2 years ago • 16 comments

As the summary says, this PR replaces es2015-i18n-tag tooling with lingui.

lingui comes with a few nice tools and workflows, but also results in SWC support for the Next application in the repo.

note: this PR doesn't include migration to SWC. lingui supports it, though.

adamsoderstrom avatar Mar 27 '23 21:03 adamsoderstrom

One thing that we still need to do before merging this, is to ensure that we provide a compiled translation file in every production build.

The production builds are currently gitignore'd in this PR (/public/locales/*/*.js). I wonder if we shall alter the yarn build script to account for this? @maeertin

adamsoderstrom avatar Mar 29 '23 11:03 adamsoderstrom

Would be very interesting to test something like this as well in our package.json:

{
   "husky": {
      "hooks": {
         "pre-commit": "lingui extract $(git diff --name-only --staged)"
      }
   }
}

source

adamsoderstrom avatar Mar 29 '23 13:03 adamsoderstrom

One thing that we still need to do before merging this, is to ensure that we provide a compiled translation file in every production build.

The production builds are currently gitignore'd in this PR (/public/locales/*/*.js). I wonder if we shall alter the yarn build script to account for this? @maeertin

I think we can skip this, tbh.

In next.js's own example of using lingui, they state:

It adds a webpack loader for the messages to avoid having to manually compile while developing as well as adds the compile step to the next build script for production builds.

Webpack loader, in this case, is @lingui/loader, i assume.

So, from my perspective, i'd say we're ready to merge, @maeertin!

adamsoderstrom avatar Apr 01 '23 06:04 adamsoderstrom

Ah, hahah! Sweet, @maeertin ! Started working on your comments from today locally, but never got to pushing them! 😄

adamsoderstrom avatar Apr 01 '23 12:04 adamsoderstrom

Is there any more testing that we might want to do before closing and mergin this PR? 🚀

Yeah. I'll add some comments to the PR and then someone (or me) can have a look on them! 😊 @maeertin

adamsoderstrom avatar Apr 01 '23 12:04 adamsoderstrom

You can now run npx @noaignite/create-app my-app --branch feature/lingui-swc to test it out in a fresh install :)

In the case that it doesn't work, you might need to delete the npx cache found at ~/.npm/_npx.

@adamsoderstrom

maeertin avatar Apr 04 '23 10:04 maeertin

You can now run npx @noaignite/create-app my-app --branch feature/lingui-swc to test it out in a fresh install :)

In the case that it doesn't work, you might need to delete the npx cache found at ~/.npm/_npx.

Worked good after removing ~/.npm/_npx, @maeertin! According to me, things looks good after building the application as well. 😊

adamsoderstrom avatar Apr 04 '23 14:04 adamsoderstrom

Are we ready for merge you think @adamsoderstrom? :)

maeertin avatar Apr 15 '23 10:04 maeertin

Are we ready for merge you think @adamsoderstrom? :)

Yeah, i guess! 😊 @maeertin

adamsoderstrom avatar Apr 17 '23 08:04 adamsoderstrom

Actually what's missing in this PR is to entirely clean up the previous solution. Don't know what remains might be in the project from it but we should at least remove the old npm packages es2015-i18n-tag and babel-plugin-i18n-tag-translate.

maeertin avatar May 02 '23 07:05 maeertin

Actually what's missing in this PR is to entirely clean up the previous solution. Don't know what remains might be in the project from it but we should at least remove the old npm packages es2015-i18n-tag and babel-plugin-i18n-tag-translate.

True. Will remove es2015-i18n-tag and babel-plugin-i18n-tag-translate. I've done some cleanup, but will look into more "leftovers"

@maeertin

adamsoderstrom avatar May 02 '23 07:05 adamsoderstrom

Also just found that babel-plugin-i18n-tag-translate is still used in babel.config.js :) @adamsoderstrom

maeertin avatar May 02 '23 07:05 maeertin

Let's also remove the old I18nContext and bump lingui to v4, which removes the need for make-plural/plurals as we discussed @adamsoderstrom

maeertin avatar May 02 '23 08:05 maeertin

There seems to be a JSX-related bug in the 4.0.0 version of Lingui. Created an issue (https://github.com/lingui/js-lingui/issues/1638)

adamsoderstrom avatar May 05 '23 08:05 adamsoderstrom

Seems like it wasn't a bug!

I've now made a following commit (e021ab609489efe10b6b208c968b31d6a407bd8c), which strives to follow @thekip's instructions. May be some refactoring to be done (use default + custom extractor, maybe).

@maeertin

adamsoderstrom avatar May 16 '23 19:05 adamsoderstrom

Guys, I will add my 50 cents here.

  1. the current state of your integration will not support Next's SSR / SSG. I didn't see any getServerSideProps / getStaticProps. Check examples: https://github.com/lingui/js-lingui/tree/main/examples and working nextjs project here https://github.com/alt3/rank-my-wallet
  2. I recommend to use <Trans>Hello {name}</Trans> whenever possible. t macro is confusing and easy to misuse.
  3. You also can leverage an automatic page level catalog splitting by using "experimental extractor", here is an example https://github.com/alt3/rank-my-wallet/pull/148 you still will have issues with JSX inside JS files, because it uses esbuild under the hood, which most likely will not understand it.

timofei-iatsenko avatar May 17 '23 08:05 timofei-iatsenko

Closing PR as "cia" is now purely a documentation app.

maeertin avatar Aug 23 '24 12:08 maeertin