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

Text translations retrieved at global scope don't return translated value #4329

Closed
fluiddot opened this issue Dec 2, 2021 · 12 comments · Fixed by #4332
Closed

Text translations retrieved at global scope don't return translated value #4329

fluiddot opened this issue Dec 2, 2021 · 12 comments · Fixed by #4332
Assignees
Labels
[Type] Bug Something isn't working

Comments

@fluiddot
Copy link
Contributor

fluiddot commented Dec 2, 2021

Description

Most of the strings marked as untranslated are actually retrieving the translated text, and their translations are included in the i18n cache files, however, the translation is not displayed.

Example:

Screenshot 2021-12-02 at 17 18 17

Screenshot 2021-12-02 at 17 17 00

As you can see, in this case, the translation of the string is retrieved at global scope in the BlockAlignmentUI component. Therefore, only if the locale data was set prior to the importation of this file, the translation won't be available.

Specifically, the file of the BlockAlignmentUI component (reference) is one of the files imported during initialization, here is the import trace:

  1. On the native editor initialization, packages/react-native-editor/src/index.js#gutenbergSetup imports @wordpress/edit-post package.
  2. packages/edit-post/src/index.native.js imports Editor component.
  3. packages/edit-post/src/editor.native.js imports EditorProvider component from @wordpress/editor package.
  4. packages/editor/src/components/provider/index.js imports BlockEditorProvider component from @wordpress/block-editor package.
  5. packages/block-editor/src/index.js imports hooks.
    NOTE: Instead of checking the BlockEditorProvider component file, we focus on the @wordpress/block-editor package index as it's the entry point when importing any module under the package.
  6. packages/block-editor/src/hooks/index.native.js imports align hook (native version).
  7. packages/block-editor/src/hooks/align.native.js imports align hook (web version).
  8. packages/block-editor/src/hooks/align.js imports BlockAlignmentControl component.
  9. packages/block-editor/src/components/block-alignment-control/index.js imports BlockAlignmentUI component.
  10. packages/block-editor/src/components/block-alignment-control/ui.js retrieves the Align left translation.

This means that the above code is executed before setting the locale data in this line hence, the translation for Align left is not available at that moment.

Potential solutions

In the following sections, I describe different solutions that we could apply. The first ones involve isolating the imports required to initialize the editor, so we can guarantee that we can set the locale data before any code related to the editor is executed.

1 - Refactor initialization code

I noticed that this is not a problem in the web version because the locale data is set before the editor is initialized. I haven't checked further but my impression is that this is done by the backend part of WordPress.

Screenshot 2021-12-02 at 17 59 21

In our case, the backend is the native code but we can't call the Javascript side until it's initialized. Besides, we're already handling initialization code in the files located in the react-native-editor package, as well as in the Gutenberg Mobile files.

The entry point in the Javascript side for React Native is when the app is registered in the AppRegistry library (reference), this part is handled by the render function located in the @wordpress/element package (reference) and is called by the initializeEditor function (reference).

A potential approach would be to move the code related to the AppRegistry to the initialization code in the react-native-editor package, which at the same time, matches the expected behavior of being this package the contact point with React Native. This way we can import and set the locale data when React Native initializes Javascript and before importing the files for initializing the editor.

Nevertheless, this approach has the following critical downsides:

  • Refactor the initialization code is not trivial and would require deep testing.

2 - Download translations from main apps GlotPress projects

This approach implies that we no longer rely on the translations object passed from the native side via the initial props (reference), since we'll download it at bundle generation time as we do with Gutenberg translations. This way we can import the translations and set the locale data before importing the files to initialize the code, assuring that translations are ready when they're retrieved at global scope.

Nevertheless, this approach has the following critical downsides:

  • When introducing new strings, we don't have a way to get their translations after cutting a release. Usually, we request translations for new strings via the localization strings files of the main apps, so unless we generate a new JS bundle at the end of the release cycle (once the translations are ready), we won't have the translations until the next release version.

3 - Modify source code files

As a last alternative, we could modify the files that retrieve the translation text at global scope, and move the code to functions or component instantiation to prevent this issue.

Nevertheless, this approach has the following critical downsides:

  • We would require updating several files and most likely introducing a native version just for this case.
  • This option won't prevent that in the future we implement new code that follows the same behavior.

To Reproduce

  1. Change the device's language to non-English.
  2. Open the app.
  3. Select a Paragraph block.
  4. Tap on the alignment button located in the toolbar.
  5. Observe that the alignment options are not translated.

Expected behavior

All strings should be translated.

Screenshots

Smartphone (please complete the following information):

  • Device: iPhone 12 Pro Max (simulator)
  • OS: iOS 15.0
  • Version N/A
@fluiddot fluiddot added the [Type] Bug Something isn't working label Dec 2, 2021
@fluiddot fluiddot self-assigned this Dec 2, 2021
@fluiddot
Copy link
Contributor Author

fluiddot commented Dec 3, 2021

Regarding option 1 (Refactor initialization code), I created the following PRs as a proof-of-concept. As far as I tested, the approach works nice so we might consider it a valid workaround.

@dcalhoun
Copy link
Member

dcalhoun commented Dec 3, 2021

💯 Great overview and thorough research, @fluiddot. Thank you for putting this together. 🙇🏻

1 - Refactor initialization code

[...]
A potential approach would be to move the code related to the AppRegistry to the initialization code in the react-native-editor package, which at the same time, matches the expected behavior of being this package the contact point with React Native. This way we can import and set the locale data when React Native initializes Javascript and before importing the files for initializing the editor.
[...]

  • Refactor the initialization code is not trivial and would require deep testing.

At a high-level, Approach 1 makes sense. One question I have is whether the "lazy loading" of modules will have any impact on load time. Do you have any insight into this so far?

I will be interested to hear whether others know of a specific reason the code is structured the way it is currently. My guess is that it is largely legacy structure preceding the monorepo merge. It may have been left in its current form to avoid the heavy testing. While more testing is a cost we would need to pay, relocating "universal" set up code into Gutenberg makes sense. I.e. translations of strings found in Gutenberg isn't all that related to Gutenberg Mobile or the WordPress mobile apps.

2 - Download translations from main apps GlotPress projects

This approach implies that we no longer rely on the translations object passed from the native side via the initial props (reference), since we'll download it at bundle generation time as we do with Gutenberg translations.
[...]

  • When introducing new strings, we don't have a way to get their translations after cutting a release. Usually, we request translations for new strings via the localization strings files of the main apps, so unless we generate a new JS bundle at the end of the release cycle (once the translations are ready), we won't have the translations until the next release version.

If I understand correctly, Approach 2 would partially detach Gutenberg's dependence upon the WordPress mobile apps for translation strings. If so, one reason to favor this might be to make it easier to integrate the Block Editor into a different mobile app.

Is it worthwhile considering a GlotPress project specifically Gutenberg that is integrated into the Gutenberg Mobile release process? Does Gutenberg for web have its own GlotPress project or does it receive translations from a higher-level WordPress GlotPress project?

3 - Modify source code files

[...]

  • We would require updating several files and most likely introducing a native version just for this case.
  • This option won't prevent that in the future we implement new code that follows the same behavior.

The brittleness of Approach 3 is definitely concerning. Additionally, I believe some assignments are hoisted out of components for performance optimization reasons. It may not be a large concern, but I wonder if we would introduce performance degradations by relocating this into React function components.

@hypest
Copy link
Contributor

hypest commented Dec 7, 2021

Thanks for the analysis @fluiddot , and for the commenting @dcalhoun!

1 - Refactor initialization code

I believe this is the minimum we should do. That is, initialization of the translation facility needs to happen before the code that uses translations runs.

I will be interested to hear whether others know of a specific reason the code is structured the way it is currently.

👋 @dcalhoun , I think you're correct that it's mostly a legacy thing. From the beginning, we were piggybacking on the main apps' translation pipeline and we thought that just passing the translation strings down early from the native side is good enough.

relocating "universal" set up code into Gutenberg makes sense. I.e. translations of strings found in Gutenberg isn't all that related to Gutenberg Mobile or the WordPress mobile apps.

The only bit I'd add is that the set of strings can indeed be related to the main app or "gutenberg-mobile" when we factor in Jetpack strings which are not available via the Gutenberg repo. So, we need a structure that "compiles" strings from different sources at different levels, kinda.

2 - Download translations from main apps GlotPress projects

My understanding is that "Solution 2" is complementary to Solution 1. That is, we should still make sure translations are initialized before the code that uses it, but when and where we'll get the strings is independent. Am I missing some detail? 🤔

When introducing new strings, we don't have a way to get their translations after cutting a release. Usually, we request translations for new strings via the localization strings files of the main apps, so unless we generate a new JS bundle at the end of the release cycle (once the translations are ready), we won't have the translations until the next release version.

That's the situation with the WP native apps as well anyway. New strings are merged and a release is cut, but since it takes time for the strings to be translated, there's a subsequent step to update the release, closer to the publication time. We could follow the same pattern and either recreate the app bundle or figure out a way to split the bundle into two, one for the app itself and one for the translated string objects, assuming we can combine them closer to the publication of the release or in runtime.

Is it worthwhile considering a GlotPress project specifically Gutenberg that is integrated into the Gutenberg Mobile release process?

I think that's the topic of this internal translations-related discussion: pxLjZ-6Fc-p2

Does Gutenberg for web have its own GlotPress project or does it receive translations from a higher-level WordPress GlotPress project?

Indeed, Gutenberg has its own GlotPress project. For instance, here's the spanish translations one.

3 - Modify source code files

That sounds like an last resort or a "just fix it for today" option. We should only consider it last.

@hypest hypest added this to Triage in Mobile Gutenberg via automation Dec 7, 2021
@hypest hypest moved this from Triage to In progress in Mobile Gutenberg Dec 7, 2021
@dcalhoun
Copy link
Member

dcalhoun commented Dec 7, 2021

The only bit I'd add is that the set of strings can indeed be related to the main app or "gutenberg-mobile" when we factor in Jetpack strings which are not available via the Gutenberg repo. So, we need a structure that "compiles" strings from different sources at different levels, kinda.

Ah, great point. I agree an ideal architecture would enable composing from different sources. E.g. Gutenberg itself, Jetpack, host app. We may ultimately apply both Approach 1 and Approach 2.

We could follow the same pattern and either recreate the app bundle or figure out a way to split the bundle into two, one for the app itself and one for the translated string objects, assuming we can combine them closer to the publication of the release or in runtime.

This crossed my mind as well and makes sense. An alpha release could capture the strings to be translated and serve as the initial release integrated in to the host app. A final release could be generated once the translation strings are finalized.

@fluiddot
Copy link
Contributor Author

fluiddot commented Dec 9, 2021

One question I have is whether the "lazy loading" of modules will have any impact on load time. Do you have any insight into this so far?

Good question, I haven't benchmarked the lazy loading but it would be interesting to measure the impact on load time. I don’t expect a big change here as we’re already following this approach when initializing the editor (reference), my approach on the refactor would be just relocating the lazy load to a different place (reference).

Is it worthwhile considering a GlotPress project specifically Gutenberg that is integrated into the Gutenberg Mobile release process? Does Gutenberg for web have its own GlotPress project or does it receive translations from a higher-level WordPress GlotPress project?

We currently rely on two/three GlotPress projects for the translations:

  • Gutenberg GlotPress project: Contains the strings used by the web and native version of the editor.
  • WordPress GlotPress projects (iOS and Android): Contain strings only used by the native version of the editor, so far there’s no difference between projects but we might have it if we introduce any platform-specific string. As a side note, they are incorporated via the localization strings files bundle/ios/GutenbergNativeTranslations.swift and bundle/android/strings.xml.

I agree that eventually, we might consider having a specific GlotPress project for Gutenberg Mobile (not sure if this option has been already considered). But it's important to note that these projects are translated by translation contributors, so it might be difficult to attract contributors for a new project.

My understanding is that "Solution 2" is complementary to Solution 1. That is, we should still make sure translations are initialized before the code that uses it, but when and where we'll get the strings is independent. Am I missing some detail? 🤔

That's right, both options require set up translations before initializing the editor. However on the latter, since we won't require to fetch the extra translations coming from the initial props (reference), the refactor would be smaller and simpler, as we can leave the app registration code in the @wordpress/edit-post package (reference).

That's the situation with the WP native apps as well anyway. New strings are merged and a release is cut, but since it takes time for the strings to be translated, there's a subsequent step to update the release, closer to the publication time. We could follow the same pattern and either recreate the app bundle or figure out a way to split the bundle into two, one for the app itself and one for the translated string objects, assuming we can combine them closer to the publication of the release or in runtime.

Yep, we could provide a flow for that stage when the translations are ready. I'd like to explore further how we do it for the main apps, in case we could expand it to include GB-mobile. Besides, I want also to verify where the translation data is fetched from, the JS bundle or the JSON files we have include as bundle assets. Depending on this matter, it would be easier to integrate it into the release pipeline.

@dcalhoun
Copy link
Member

dcalhoun commented Dec 9, 2021

I don’t expect a big change here as we’re already following this approach when initializing the editor (reference), my approach on the refactor would be just relocating the lazy load to a different place (reference).

Thanks for pointing this out. I agree the changes to this likely have little-to-no impact given are merely relocating the lazy-loading of the module.

I agree that eventually, we might consider having a specific GlotPress project for Gutenberg Mobile (not sure if this option has been already considered). But it's important to note that these projects are translated by translation contributors, so it might be difficult to attract contributors for a new project.

I appreciate you providing the additional details of the current setup. It is a good point that attracting translation contributors would be a big undertaking.

@fluiddot
Copy link
Contributor Author

fluiddot commented Dec 10, 2021

Besides, I want also to verify where the translation data is fetched from, the JS bundle or the JSON files we have include as bundle assets. Depending on this matter, it would be easier to integrate it into the release pipeline.

I explored this further and noticed that, although the JSON files referenced in the code (reference) are exported as assets (reference) when generating the bundle, the source of the data is the JS bundle and not the asset files.

Looks like this is a known bug in the metro CLI that considers JSON files as assets by default when it shouldn't leading to duplicated files. This issue is already fixed and will be available when upgrading to RN 0.66 (reference), as it includes a bump to a recent version of @react-native-community/cli and metro.

@dcalhoun I'm wondering if you encountered this when generating the JS bundle when working on the RN 0.66 upgrade.

@dcalhoun
Copy link
Member

@dcalhoun I'm wondering if you encountered this when generating the JS bundle when working on the RN 0.66 upgrade.

@fluiddot I did not specifically review how the JSON files were managed, but I did not encounter any oddities. From light testing of i18n just now, it would appear that translations are displayed as expected within the RN 0.66 upgrade work. Is there something specific you feel I should review or test?

@fluiddot
Copy link
Contributor Author

@dcalhoun I wouldn't expect any issue regarding translations, at least from what I saw in the 0.66 changelog. The only change we probably will experience is that JSON files are no longer generated as asset files when producing a bundle, so we could remove all JSON files under bundle/ios/assets/ folders as won't be updated after the upgrade. I could do a quick test on the RN upgrade branch to verify it if you'd like to 👍 .

@dcalhoun
Copy link
Member

The only change we probably will experience is that JSON files are no longer generated as asset files when producing a bundle, so we could remove all JSON files under bundle/ios/assets/ folders as won't be updated after the upgrade.

@fluiddot after deleting bundle/ios/assets/gutenberg/packages/react-native-editor/i18n-cache/, running npm run bundle did not recreate the JSON files that once occupied that location. So, it would appear your inclination is correct.

I explored this further and noticed that, although the JSON files referenced in the code (reference) are exported as assets (reference) when generating the bundle, the source of the data is the JS bundle and not the asset files.

Below is a list of all the JSON files currently included in the bundle. Given your finding that the source for these assets is the JavaScript bundle, not the exported JSON file itself, I suppose it is safe and appropriate to remove the JSON asset files. WDYT?

Current JSON Files
bundle/ios/assets
├── gutenberg
│   ├── node_modules
│   │   ├── @react-navigation
│   │   │   └── stack
│   │   │       └── src
│   │   │           └── views
│   │   │               └── assets
│   │   ├── css-color-keywords
│   │   │   └── colors.json
│   │   ├── css-tree
│   │   │   ├── data
│   │   │   │   └── patch.json
│   │   │   └── package.json
│   │   ├── entities
│   │   │   └── maps
│   │   │       ├── decode.json
│   │   │       ├── entities.json
│   │   │       ├── legacy.json
│   │   │       └── xml.json
│   │   ├── mdn-data
│   │   │   └── css
│   │   │       ├── at-rules.json
│   │   │       ├── properties.json
│   │   │       └── syntaxes.json
│   │   ├── moment-timezone
│   │   │   └── data
│   │   │       └── packed
│   │   │           └── latest.json
│   │   ├── react-native-svg
│   │   │   └── node_modules
│   │   │       └── css-select
│   │   │           └── lib
│   │   │               └── procedure.json
│   │   └── react-native-url-polyfill
│   │       └── package.json
│   └── packages
│       ├── block-library
│       │   └── src
│       │       ├── archives
│       │       │   └── block.json
│       │       ├── audio
│       │       │   └── block.json
│       │       ├── block
│       │       │   └── block.json
│       │       ├── button
│       │       │   └── block.json
│       │       ├── buttons
│       │       │   └── block.json
│       │       ├── calendar
│       │       │   └── block.json
│       │       ├── categories
│       │       │   └── block.json
│       │       ├── code
│       │       │   └── block.json
│       │       ├── column
│       │       │   └── block.json
│       │       ├── columns
│       │       │   └── block.json
│       │       ├── cover
│       │       │   └── block.json
│       │       ├── embed
│       │       │   └── block.json
│       │       ├── file
│       │       │   └── block.json
│       │       ├── freeform
│       │       │   └── block.json
│       │       ├── gallery
│       │       │   └── block.json
│       │       ├── group
│       │       │   └── block.json
│       │       ├── heading
│       │       │   └── block.json
│       │       ├── html
│       │       │   └── block.json
│       │       ├── image
│       │       │   └── block.json
│       │       ├── latest-comments
│       │       │   └── block.json
│       │       ├── latest-posts
│       │       │   └── block.json
│       │       ├── list
│       │       │   └── block.json
│       │       ├── media-text
│       │       │   └── block.json
│       │       ├── missing
│       │       │   └── block.json
│       │       ├── more
│       │       │   └── block.json
│       │       ├── nextpage
│       │       │   └── block.json
│       │       ├── paragraph
│       │       │   └── block.json
│       │       ├── preformatted
│       │       │   └── block.json
│       │       ├── pullquote
│       │       │   └── block.json
│       │       ├── quote
│       │       │   └── block.json
│       │       ├── rss
│       │       │   └── block.json
│       │       ├── search
│       │       │   └── block.json
│       │       ├── separator
│       │       │   └── block.json
│       │       ├── shortcode
│       │       │   └── block.json
│       │       ├── social-link
│       │       │   └── block.json
│       │       ├── social-links
│       │       │   └── block.json
│       │       ├── spacer
│       │       │   └── block.json
│       │       ├── table
│       │       │   └── block.json
│       │       ├── tag-cloud
│       │       │   └── block.json
│       │       ├── text-columns
│       │       │   └── block.json
│       │       ├── verse
│       │       │   └── block.json
│       │       └── video
│       │           └── block.json
│       ├── blocks
│       │   └── src
│       │       └── api
│       │           └── i18n-block.json
│       ├── editor
│       │   └── src
│       │       └── components
│       │           └── editor-help
│       │               └── images
│       └── react-native-editor
│           └── i18n-cache
│               └── data
│                   ├── ar.json
│                   ├── bg.json
│                   ├── bo.json
│                   ├── ca.json
│                   ├── cs.json
│                   ├── cy.json
│                   ├── da.json
│                   ├── de.json
│                   ├── el.json
│                   ├── en-au.json
│                   ├── en-ca.json
│                   ├── en-gb.json
│                   ├── en-nz.json
│                   ├── en-za.json
│                   ├── es-ar.json
│                   ├── es-cl.json
│                   ├── es-cr.json
│                   ├── es.json
│                   ├── fa.json
│                   ├── fr.json
│                   ├── gl.json
│                   ├── he.json
│                   ├── hr.json
│                   ├── hu.json
│                   ├── id.json
│                   ├── is.json
│                   ├── it.json
│                   ├── ja.json
│                   ├── ka.json
│                   ├── ko.json
│                   ├── nb.json
│                   ├── nl-be.json
│                   ├── nl.json
│                   ├── pl.json
│                   ├── pt-br.json
│                   ├── pt.json
│                   ├── ro.json
│                   ├── ru.json
│                   ├── sk.json
│                   ├── sq.json
│                   ├── sr.json
│                   ├── sv.json
│                   ├── th.json
│                   ├── tr.json
│                   ├── uk.json
│                   ├── ur.json
│                   ├── vi.json
│                   ├── zh-cn.json
│                   └── zh-tw.json
└── jetpack
    ├── node_modules
    └── projects
        └── plugins
            └── jetpack
                └── extensions
                    ├── blocks
                    │   └── story
                    └── index.json

@fluiddot
Copy link
Contributor Author

@fluiddot after deleting bundle/ios/assets/gutenberg/packages/react-native-editor/i18n-cache/, running npm run bundle did not recreate the JSON files that once occupied that location. So, it would appear your inclination is correct.

Great! Thanks for checking it 🙇 .

Below is a list of all the JSON files currently included in the bundle. Given your finding that the source for these assets is the JavaScript bundle, not the exported JSON file itself, I suppose it is safe and appropriate to remove the JSON asset files. WDYT?

Yep, I think we can safely remove them as they are already included in the JS bundle ✂️ .

@fluiddot
Copy link
Contributor Author

One question I have is whether the "lazy loading" of modules will have any impact on load time. Do you have any insight into this so far?

@dcalhoun I did a quick benchmarking on the lazy loading of the setup code and, as far as I checked, it doesn't introduce a delay in the code. The difference between the timestamps before and after importing that code (reference) is 0 so I infer that shouldn't impact the load time.

dcalhoun added a commit that referenced this issue Dec 22, 2021
The static JSON file assets are not the sourced by the app. Instead, the
data found within the JavaScript bundle itself is utilized. The
unnecessary static JSON files should now no longer be exported when
bundling the JavaScript thanks to a bug fix in React Native.

- #4329 (comment)
- facebook/metro#593
Mobile Gutenberg automation moved this from In progress to Done (keep clean, manually) Jan 4, 2022
@mkevins mkevins mentioned this issue Jan 7, 2022
4 tasks
dcalhoun added a commit that referenced this issue Jan 24, 2022
The static JSON file assets are not the sourced by the app. Instead, the
data found within the JavaScript bundle itself is utilized. The
unnecessary static JSON files should now no longer be exported when
bundling the JavaScript thanks to a bug fix in React Native.

- #4329 (comment)
- facebook/metro#593
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
Projects
Mobile Gutenberg
  
Done (keep clean, manually)
3 participants