Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prebundle to bundle command to force update the i18n localization files #3423

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Apr 27, 2021

Fixes #3422

This PR adds the core prebundle command to the bundle command to force update the i18n localization files. This way we assure that we always bundle the most recent localization files.

To test:

  1. Run command npm run bundle
  2. Observe that the i18n localization files are updated, check the following locations:
    • gutenberg/packages/react-native-editor/i18n-cache/data/<LOCALE>.json
    • bundle/android/raw/gutenberg_packages_reactnativeeditor_i18ncache_data_<LOCALE>.json
    • bundle/ios/assets/gutenberg/packages/react-native-editor/i18n-cache/cache/<LOCALE>.json

Verify localization files are bundled into the main apps

iOS

The files are referenced via the Gutenberg podspec so in this case, if the files generated in bundle/ios/assets/gutenberg/packages/react-native-editor/i18n-cache/cache are ok then they will be bundled into WPiOS.

Android

Since now the JS bundle has to be published for injecting it into WPAndroid, we have to verify with an installable build that the localizations are included. For this purpose I created the following PR: wordpress-mobile/WordPress-Android#14535.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@fluiddot fluiddot added [Type] Bug Something isn't working [Type] Regression Existing functionality that got broken labels Apr 27, 2021
@fluiddot fluiddot added this to the 1.51.1 (17.2) milestone Apr 27, 2021
@fluiddot fluiddot requested review from antonis and mchowning April 27, 2021 11:38
@fluiddot fluiddot self-assigned this Apr 27, 2021
@fluiddot fluiddot changed the title Add prebundle to bundle command to force update the i18n localization files Add prebundle to bundle command to force update the i18n localization files Apr 27, 2021
@peril-wordpress-mobile
Copy link

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@@ -304,7 +304,9 @@ jobs:
- npm-install
- run:
name: Build JavaScript Bundle
command: npm run bundle:android
command: |
npm run core prebundle
Copy link
Contributor Author

@fluiddot fluiddot Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have to update the localization files when publishing the bundle for Android.

@@ -61,7 +61,7 @@
"prern-bundle": "patch-package --patch-dir gutenberg/packages/react-native-editor/metro-patch",
"rn-bundle": "react-native bundle",
"postrn-bundle": "patch-package --reverse --patch-dir gutenberg/packages/react-native-editor/metro-patch",
"bundle": "npm run clean; npm run bundle:js && npm run genstrings",
"bundle": "npm run clean; npm run core prebundle && npm run bundle:js && npm run genstrings",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I suppose the pre scripts we have in place (e.g. prebundle) were not correctly running because we many of our bundle scripts have suffixes (e.g. :js, :android) which causes the pre script to not execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, besides the prebundle command is defined in packages/react-native-editor and we usually run the bundle command on the root (GB-mobile).

@dcalhoun
Copy link
Member

@fluiddot I did just notice that I did not see the bundle/android/raw/gutenberg_packages_reactnativeeditor_i18ncache_data_<LOCALE>.json paths update. I wonder if that is coincidence or an issue?

bundle-changes

@fluiddot
Copy link
Contributor Author

I created this PR on WordPress-Android repo for making an installable build to test if localizations were bundled.

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 27, 2021

@fluiddot I did just notice that I did not see the bundle/android/raw/gutenberg_packages_reactnativeeditor_i18ncache_data_<LOCALE>.json paths update. I wonder if that is coincidence or an issue?

bundle-changes

Yep, that's because we're not tracking the dir bundle/android/raw/. I was also surprised about this since we're doing it for iOS but at least now that we're publishing the JS bundle for injecting it into WPAndroid, I think it's not really necessary to track them.

@fluiddot fluiddot modified the milestones: 1.51.1 (17.2), 1.52.0 (17.3) Apr 28, 2021
@fluiddot fluiddot merged commit 9a7c745 into develop Apr 28, 2021
@fluiddot fluiddot deleted the fix/update-i18n-cache branch April 28, 2021 10:14
@cameronvoell cameronvoell mentioned this pull request Apr 30, 2021
4 tasks
@hypest
Copy link
Contributor

hypest commented May 5, 2021

Thanks for the fix @fluiddot and @dcalhoun !

This PR adds the core prebundle command to the bundle command to force update the i18n localization files

I wonder if using the prebundle is not practical anymore. Forcing it feels unintuitive (since one would assume it runs automatically but it doesn't). Besides, that script only has commands about i18n currently so, maybe it's better to rename it (and force it on bundle too), just to help with false assumptions?

@dcalhoun
Copy link
Member

dcalhoun commented May 5, 2021

I wonder if using the prebundle is not practical anymore. Forcing it feels unintuitive (since one would assume it runs automatically but it doesn't). Besides, that script only has commands about i18n currently so, maybe it's better to rename it (and force it on bundle too), just to help with false assumptions?

Possibly. Now that I look at this again, I wonder if there is a way to expose fewer scripts from gutenberg up to gutenberg-mobile. By that I mean, the latter shouldn't need to call core prebundle (or the same script named differently). Ideally, running a single command from gutenberg (e.g. bundle:android) would cause gutenberg to manage the dependencies it requires for the given command (e.g. internationalization strings).

One thing I do not fully understand currently is why both gutenberg and gutenberg-mobile have scripts that appear to be identical (e.g. bundle:android). I'd need to further assess all of the scripts before I could provide a holistic opinion on the matter. I will try to make time to do so.

@fluiddot
Copy link
Contributor Author

fluiddot commented May 5, 2021

I wonder if using the prebundle is not practical anymore. Forcing it feels unintuitive (since one would assume it runs automatically but it doesn't). Besides, that script only has commands about i18n currently so, maybe it's better to rename it (and force it on bundle too), just to help with false assumptions?

I don't have a strong opinion whether remove or not the prebundle command, we're only executing one command but at the same time if in the future we need to run more before bundling we would need it.

I'd like to note that generating the bundle within gutenberg via the react-native-editor package, generates the strings and bundles them as expected. The problem was generating the bundle from gutenberg-mobile wasn't taking into account that it requires running extra commands beforehand. Being this said, I see two options to apply in gutenberg-mobile:

  1. Instead of running npm run core prebundle when bundling, run npm run core i18n-cache:force
  2. Add the command prebundle to gutenberg-mobile that would run npm run core i18n-cache:force

Possibly. Now that I look at this again, I wonder if there is a way to expose fewer scripts from gutenberg up to gutenberg-mobile. By that I mean, the latter shouldn't need to call core prebundle (or the same script named differently). Ideally, running a single command from gutenberg (e.g. bundle:android) would cause gutenberg to manage the dependencies it requires for the given command (e.g. internationalization strings).

I had the same impression but as far as I understand they're generating different bundles. What I also do wonder is how many of them are doing the same and could unified 🤔 .

One thing I do not fully understand currently is why both gutenberg and gutenberg-mobile have scripts that appear to be identical (e.g. bundle:android). I'd need to further assess all of the scripts before I could provide a holistic opinion on the matter. I will try to make time to do so.

They're but have a different entry point:

  1. gutenberg-mobile points to index.js from gutenberg-mobile
  2. gutenberg points to index.js from gutenberg/packages/react-native-editor

@dcalhoun
Copy link
Member

dcalhoun commented May 6, 2021

I don't have a strong opinion whether remove or not the prebundle command, we're only executing one command but at the same time if in the future we need to run more before bundling we would need it.

Agreed. I believe either approach is reasonable.

Possibly. Now that I look at this again, I wonder if there is a way to expose fewer scripts from gutenberg up to gutenberg-mobile. By that I mean, the latter shouldn't need to call core prebundle (or the same script named differently). Ideally, running a single command from gutenberg (e.g. bundle:android) would cause gutenberg to manage the dependencies it requires for the given command (e.g. internationalization strings).

I had the same impression but as far as I understand they're generating different bundles. What I also do wonder is how many of them are doing the same and could unified 🤔 .

One thing I do not fully understand currently is why both gutenberg and gutenberg-mobile have scripts that appear to be identical (e.g. bundle:android). I'd need to further assess all of the scripts before I could provide a holistic opinion on the matter. I will try to make time to do so.

They're but have a different entry point:

  1. gutenberg-mobile points to index.js from gutenberg-mobile
  2. gutenberg points to index.js from gutenberg/packages/react-native-editor

Ah, yes. They do appear to be quite different. Likely not worth pursuing alignment or consideration at this time.

@fluiddot
Copy link
Contributor Author

As a cross-reference, I updated the prebundle command on GB-mobile in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working [Type] Regression Existing functionality that got broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block titles are not translated
3 participants