Page MenuHomePhabricator

[cashtab] Internationalization: Switch from fbt to react-intl
AbandonedPublic

Authored by alcipir on Jul 9 2021, 03:52.

Details

Reviewers
majcosta
Group Reviewers
Restricted Project
Summary

Replace fbt with formatjs (react-intl), which is easier to use in the context of a React App.

Test Plan

Run the following commands:

npm install
npm run test
npm start

Then, check if texts are in english.
Switch language/locale to portuguese (Brazil). Restart the server and check if texts are in portuguese.
Switch to a different language that is not supported (not english or portuguese) and check if texts are in english (default).

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashtab-switch-to-react-intl
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16202
Build 32273: Build Diff
Build 32272: arc lint + arc unit

Event Timeline

alcipir requested review of this revision.Jul 9 2021, 03:52
majcosta requested changes to this revision.Jul 9 2021, 12:39
majcosta added a subscriber: majcosta.
  • extension/src/components/App.js still has fbt imports and tags
  • .gitignore still has an fbt related file section
  • test plan doesn't test all changes
web/cashtab/package.json
173 ↗(On Diff #29122)

missed this guy

This revision now requires changes to proceed.Jul 9 2021, 12:39

Removed fbt from extension and removed remaining fbt packages.

Removed fbt-related files on .gitignore

Tail of the build log:

/work/web/cashtab /work/abc-ci-builds/cashtab-tests

added 2816 packages, and audited 2817 packages in 43s

151 packages are looking for funding
  run `npm fund` for details

14 vulnerabilities (13 moderate, 1 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
npm notice 
npm notice New minor version of npm available! 7.7.6 -> 7.20.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v7.20.0>
npm notice Run `npm install -g npm@7.20.0` to update!
npm notice 

> cashtab@1.0.0 prebuild
> npm run compile


> cashtab@1.0.0 compile
> formatjs compile-folder --ast --format transifex lang src/compiled-lang


> cashtab@1.0.0 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

./src/components/App.js
Cannot find module: '@assets/popout.svg'. Make sure this package is installed.

You can install this package by running: npm install @assets/popout.svg.


Build cashtab-tests failed with exit code 1

looks like something broke the tests, back to your queue

majcosta requested changes to this revision.Jul 16 2021, 18:14
This revision now requires changes to proceed.Jul 16 2021, 18:14

Fixing overwritten files from extension script.

Tail of the build log:

/work/web/cashtab /work/abc-ci-builds/cashtab-tests

added 2816 packages, and audited 2817 packages in 42s

151 packages are looking for funding
  run `npm fund` for details

14 vulnerabilities (13 moderate, 1 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
npm notice 
npm notice New minor version of npm available! 7.7.6 -> 7.20.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v7.20.0>
npm notice Run `npm install -g npm@7.20.0` to update!
npm notice 

> cashtab@1.0.0 prebuild
> npm run compile


> cashtab@1.0.0 compile
> formatjs compile-folder --ast --format transifex lang src/compiled-lang


> cashtab@1.0.0 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

./src/components/App.js
Cannot find module: 'fbt'. Make sure this package is installed.

You can install this package by running: npm install fbt.


Build cashtab-tests failed with exit code 1

Looking at react-intl, it seems to me like it is a significant step backward.

The first obvious one is the absurd number of dependencies. But more importantly, it seems to break composability:

<fbt desc="foobar">
    You can read more about this very interesting topic on <a href="https://john.blog/">John's blog</a> or by consulting the <a href="https://foobar.archive/">foobar archive</a>.
</fbt>

Now that would seem like you could splice and dice this, but you really can't because you don't know if the structure of the sentence will look the same in another language.

There are also no support for genders.

I'm arguably no specialist and may have missed some stuff, but as far as I can see, react-intl did happily jumped into all the traps most intl framework happily jump into and learned nothing from it.

Please apply changes to the non-extension version of App.js as well

Regarding fbt vs react-intl -- fbt is probably better. However, fbt is creating a number of dev ops issues (npm start throwing warnings, errors).

I think the best approach would be

  1. Remove all localization and all scripts related to localization
  2. Upgrade to webpack 5
  3. Evaluate and implement best localization software

However, I think it's okay to drop in replace fbt with react-intl for now (since it's already done in this diff). If this solves the fbt issues, it's a step in the right direction. However, I think we will still need to remove all localization and make all of the build and dev scripts as simple as possible before upgrading to webpack 5.

In D9760#222098, @josephroyking wrote:

If this solves the fbt issues, it's a step in the right direction.

Without specifying what these issues are, there is no way to know it is a step in the right direction.

Good point. @alcipir please abandon this diff. We should start with no localization to simplify the dependency tree for migration to webpack 5.