search

wordpress-mobile / gutenberg-mobile

Mobile version of Gutenberg - native iOS and Android
GNU General Public License v2.0
241 stars 55 forks source link

build: Fix i18n script paths causing `bundle` command not found errors #6700

Closed dcalhoun closed 6 months ago

dcalhoun commented 6 months ago

Description

Certain React Native CLI commands were relocated when cli-plugin-metro was moved to the React Native core repository.

https://github.com/react-native-community/cli/commit/80c84d019bc2d3b640c03856e7111a6459e62357

This resulted in an inability for gutenberg-mobile project scripts to locate commands like bundle, as the react-native package is not installed as direct dependency. Instead gutenberg-mobile relies upon the version within the gutenberg Git submodule.

This change relies upon changing directories into the Git submodule so that Node.js module resolution locates the React Native package.

Similar changes were applied to other React Native CLI commands during the React Native 0.73 upgrade.

https://github.com/wordpress-mobile/gutenberg-mobile/commit/e2e5429805fe199f97df58f2dfc9872ac75297dc

Testing Instructions

  1. Run npm run i18n:update.
  2. Verify it succeeds and expected files are generated.

PR submission checklist:

dcalhoun commented 6 months ago

My thought is that we should verify the i18n features continue working as expected after this change, but that there is likely little risk merging this in the meantime to repair CI builds and finish integrating into the host apps.

@fluiddot I requested your review as I perceive you are fairly familiar with the i18n scripts. I lack confidence as to which files I should verify are created. I'm going to continue researching, but let me know if there are key artifacts I should verify. Thanks! 🙇🏻

dcalhoun commented 6 months ago

From reading the i18n docs (which are great) and testing the outcome of npm run bundle, my thought is that the proposed changes are stable. I did not observe any unexpected string removals from the generated bundle files.

fluiddot commented 6 months ago

@fluiddot I requested your review as I perceive you are fairly familiar with the i18n scripts. I lack confidence as to which files I should verify are created. I'm going to continue researching, but let me know if there are key artifacts I should verify. Thanks! 🙇🏻

Sounds good, I can take a look. The artifacts to check are the localization strings files:

We can test adding two strings in Gutenberg to verify if they are updated:

  1. Add a native-only string. This string should be included in the localization strings files.
  2. Add a string already used in the web version. This string shouldn't be included in the localization strings files.

Apart from this, we could optionally check the intermediate files generated in the i18 update process. But this requires tweaking the code/using different CLI parameters to avoid removing those files as are temporary (example).

UPDATE: We could use the npm command i18n:update:test to check the intermediate files.

dcalhoun commented 6 months ago

Thank you for the guidance, testing, and review! 🙇🏻

Merging this to keep the React Native upgrade moving forward. I'll continue testing the artifacts and address any issues noted in a separate PR.

dcalhoun commented 6 months ago

I verified both of the following cases function as expected:

We can test adding two strings in Gutenberg to verify if they are updated:

  1. Add a native-only string. This string should be included in the localization strings files.
  2. Add a string already used in the web version. This string shouldn't be included in the localization strings files.

I also verified intermediate file generation using i18n:update by inspecting the temporary directory during script execution.

Apart from this, we could optionally check the intermediate files generated in the i18 update process. But this requires tweaking the code/using different CLI parameters to avoid removing those files as are temporary (example).

UPDATE: We could use the npm command i18n:update:test to check the intermediate files.

It appears this change does break the i18n:update:test command. I will fix that issue in a separate PR.

i18n:update:test error ``` Success: POT file successfully generated. Download I18n translations for "jetpack" plugin node:internal/modules/cjs/loader:1147 throw err; ^ Error: Cannot find module '/Users/[REDACTED]/gutenberg-mobile/i18n-test/used-strings.json' Require stack: - /Users/[REDACTED]/gutenberg-mobile/gutenberg/packages/react-native-editor/bin/i18n-translations-download.js at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15) at Module._load (node:internal/modules/cjs/loader:985:27) at Module.require (node:internal/modules/cjs/loader:1235:19) at require (node:internal/modules/helpers:176:18) at getUsedStrings (/Users/[REDACTED]/gutenberg-mobile/gutenberg/packages/react-native-editor/bin/i18n-translations-download.js:239:22) at Object. (/Users/[REDACTED]/gutenberg-mobile/gutenberg/packages/react-native-editor/bin/i18n-translations-download.js:289:5) at Module._compile (node:internal/modules/cjs/loader:1376:14) at Module._extensions..js (node:internal/modules/cjs/loader:1435:10) at Module.load (node:internal/modules/cjs/loader:1207:32) at Module._load (node:internal/modules/cjs/loader:1023:12) { code: 'MODULE_NOT_FOUND', requireStack: [ '/Users/[REDACTED]/gutenberg-mobile/gutenberg/packages/react-native-editor/bin/i18n-translations-download.js' ] } Node.js v20.10.0 ```