noaignite / create-ignite-app

Boilerplate for React with Next.js and MUI
17 stars 2 forks source link

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

Closed adamsoderstrom closed 2 weeks ago

adamsoderstrom commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

adamsoderstrom commented 1 year ago

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

maeertin commented 1 year ago

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

adamsoderstrom commented 1 year ago

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. 😊

maeertin commented 1 year ago

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

adamsoderstrom commented 1 year ago

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

Yeah, i guess! 😊 @maeertin

maeertin commented 1 year ago

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.

adamsoderstrom commented 1 year ago

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

maeertin commented 1 year ago

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

maeertin commented 1 year ago

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

adamsoderstrom commented 1 year ago

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 commented 1 year ago

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

thekip commented 1 year ago

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.
maeertin commented 2 weeks ago

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