Translation files added to prod bundle #551

ataylorme · 2019-07-12T03:31:40Z

Description

Addresses issue #512.

Copy .pot, .po, and .mo files to the prod theme
Use wp-cli to generate the .pot file instead of gulp-pot.
Copied translation files include string replacement.
Refactor gulp tests and add coverage for translations and prod string replace files.

List of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

This pull request relates to a ticket.
This code is tested.
This change has been added to CHANGELOG.md
I want my code added to WP Rig.

- Use wp-cli to generate the `.pot` file instead of `gulp-pot`. - Copied translation files include string replacement - Refactor gulp tests and add coverage for translations and prod string replace files.

benoitchantre · 2019-07-12T20:32:15Z

Thank you @ataylorme for your work on this.

Here are some comments:

Existing .json files should also be copied to the prod theme, they are required to translate internationalized strings in .js files. I think string replacements can be skipped for these files.
.mo files should not be copied to the prod theme but should be generated from the rewritten .po files in the production as a last step

Here's an example of what produces wp i18n make-json on PR #379:

{"translation-revision-date":"YEAR-MO-DA HO:MI+ZONE","generator":"WP-CLI\/2.2.0","source":"assets\/js\/src\/editor\/editor-filters.js","domain":"messages","locale_data":{"messages":{"":{"domain":"messages","lang":"fr","plural-forms":"nplurals=2; plural=(n != 1);"},"Checkmark":["Case \u00e0 cocher"]}}}

ataylorme · 2019-07-12T20:52:29Z

.mo files should not be copied to the prod theme but should be generated from the rewritten .po files in the production as a last step

I think gettext-parser will be needed for this. It has methods like po.parse and mo.compile so I can source a .po file and convert it to .mo mid-stream, then save it.

gulp-i18next-conv also looks interesting. It converts .po files to the newer .json standard.

@benoitchantre for WP Rig do you think we can support generating .json only or are .po and .mo necessary?

benoitchantre · 2019-07-12T21:57:28Z

gulp-i18next-conv also looks interesting. It converts .po files to the newer .json standard.

There's also a WP-CLI command for that: wp i18n make-json languages --no-purge
I think WP-CLI should be preferred as it is used by translate.wordpress.org.

for WP Rig do you think we can support generating .json only or are .po and .mo necessary?

When the theme is distributed on wp.org, everything is handled by translate.wordpress.org

When the theme is private, all these files are needed

.pot: used by translators as a template to create .po, contains source strings only
.po: used by translators, contains source strings and their translations
.mo: used by WordPress, not human readable, compiled from .po
.json: used by WordPress, contains strings extracted from .po

If we want to be able to test translations in .js in the dev theme, the .json files they need to be generated, but this can be done using WP-CLI directly. WP Rig could provide a wrapper command if we think it is a good idea.

When we run npm run bundle, the .json files need to be generated from the .po files because their filename will include an md5 hash of the source file (different between dev theme and prod theme).

ataylorme · 2019-07-18T00:13:38Z

wp i18n make-json languages --no-purge isn't working in the gulp translate task. I've also called it manually with .po files in languages and it didn't work as expected.

Aside from no .json generation, I think this is functioning. I'll probably need your help to test @benoitchantre.

I wrote some test cases to make sure the production task runs as expected.

I need to write some more test cases. If something isn't working for translations I can add more test cases for that here but otherwise, I think additional cases for the prod build, like making sure PHP files are copied and string replacement has been run, can be a follow-up issue/PR.

ataylorme · 2019-07-18T00:20:17Z

For the tests, I tried using the beforeAll and afterAll feature in Jest to do some set up (e.g. copying the .po and .mo files ) and running the gulp bundle task. After struggling for a few days, I switched strategies and set up Node scripts for the setup and teardown and used npm-run-all to run those before and after the prod tests.

The other tests that @phated helped me set up on gulp office hours test one task/function but the bundle task is different since it is a lot of other tasks in series/parallel and that is hard to mock. So in this case, rather than mocking, I am actually scaffolding real files, running the gulp bundle task, running the tests, then tearing down.

ataylorme · 2019-07-18T00:23:35Z

Actually, I take back that this is all done except one thing

.mo files should not be copied to the prod theme but should be generated from the rewritten .po files in the production as a last step

.mo files are copied right now. I'll need to research how to generate them from gulp after the .po files have been copied to the prod directory

benoitchantre · 2019-07-18T16:39:13Z

@ataylorme You can probably use gulp-potomo, which uses the msgfmt command. msgfmt is mentioned in the Localization section of the Theme Handbook

- Properly generate `.mo` files with the new gulp task `generateMOfiles` - Test for `.json` files created from `.po` files when doing the production build

ataylorme · 2019-07-20T18:39:48Z

Do you have a .po file I can test with that has JavaScript strings in it?

@benoitchantre never mind, I was able to use the WordPress core French .po file for version 5.1 and it had lots of .js strings.

wp i18n make-json languages --no-purge now generates .json files from those for testing. There is a separate JSON file generated for each source .js file the .po file includes. This made testing a bit tricky as I could not predict the hash in the generated JSON file names but I got it sorted out.

I think this is ready for final review/testing and then can be merged. Definitely, a more complex workflow and I am glad I was able to write tests to know all the steps are working as intended.

benoitchantre · 2019-07-20T20:03:57Z

Thank you for your work on this @ataylorme. I’ll test that in the next few days.

benoitchantre · 2019-07-22T12:07:16Z

@ataylorme Thank you for your work on this. This is a complicated workflow.

Here's the results of my tests:

npm run translate
The.pot file is now generated by WP-CLI. Unfortunately, I don't get the expected strings from a JS file that I get when I run wp i18n make-pot directly. It is certainly related with the exclude parameter (assets/js/src/ is excluded): the minified files don't contain __() but something transformed, like label:e(). WP-CLI cannot find i18n functions in the minified files.
To make some tests with i18n in JS, you can use editor-filters.js from #379 which uses __(). Checkmarkshould be part of the .pot file.

npm run build
I get many errors from PHP Code Sniffer: Mismatched text domain. Expected 'wp-rig' but got 'prod-theme'.

npm run bundle
First attempt: the process stopped at generateMOfiles, because gettext was not installed on my computer.
Second attempt after having installed gettext: .pot, .po, .mo and .json files are in the prod theme with the expected content.

I still need to do more tests with .json files to see if they are loaded. My guess is that it will not work because .json file names contain an MD5 hash based on the script file name. If we use the script from the src directory to extract the strings but load the minified scripts, WordPress will not find a corresponding MD5 hash. Either i18n functions should be blacklisted form minification or we should rename .json files to use the script handle instead of the MD5 hash.

.json file names can be named like this :

$textdomain-$locale-$handle.json
$textdomain-$locale-$md5.json (MD5 hash of the JavaScript file name including the extension)*

Interesting reading: Internationalization in WordPress 5.0 from Pascal Birchler.

ataylorme · 2019-07-22T14:20:01Z

Either i18n functions should be blacklisted form minification or we should rename .json files to use the script handle instead of the MD5 hash.

We are using uglify to minify scripts. You cannot white list specific functions but there is an option keep_fnames which will keep all function names from being transformed. This will increase the size of the minified JS.

I think I will try to remove assets/js/src from the exclusion and renaming the JSON files first.

Second attempt after having installed gettext

We will need to document the gettext requirement for translations. I think we will need a doc on translations anyway.

Right now there is a config option export.generatePotFile that defaults to true. Since we are now creating .mo and .json files as well perhaps it makes sense to rename it to export.generateTranslationFiles.

I have been thinking about this and do not know if we should enable all the translation functionality in the default build task. Instead, we can keep translate callable on its own during development and use the option above to toggle it for production.

ataylorme · 2019-07-22T14:22:22Z

npm run build
I get many errors from PHP Code Sniffer: Mismatched text domain. Expected 'wp-rig' but got 'prod-theme'.

I don't get these errors. This pull request doesn't edit any .php files so not sure how that got changed for you.

npm run build                                                                                                             07/22/19 at 7:18:03

> [email protected] build /Users/andrewtaylor/Sites/wprig/wp-content/themes/wprig
> gulp buildDev

[07:20:06] Requiring external module @babel/register
[07:20:09] Using gulpfile ~/Sites/wprig/wp-content/themes/wprig/gulpfile.babel.js
[07:20:09] Starting 'buildDev'...
[07:20:09] Starting 'php'...
[07:20:09] Starting 'images'...
[07:20:09] Starting 'scripts'...
[07:20:09] Starting 'translate'...
[07:20:09] Starting 'generateMOfiles'...
[07:20:09] Starting 'styles'...
[07:20:10] Finished 'generateMOfiles' after 825 ms
> "vendor/wp-cli/wp-cli/bin/wp" 'i18n' 'make-pot' '/Users/andrewtaylor/Sites/wprig/wp-content/themes/wprig' '/Users/andrewtaylor/Sites/wprig/wp-content/themes/wprig/languages/wp-rig.pot' '--exclude=vendor,node_modules,.git,gulp,tests,config,assets/js/src,assets/css/src,optional'
> "vendor/wp-cli/wp-cli/bin/wp" 'i18n' 'make-json' '/Users/andrewtaylor/Sites/wprig/wp-content/themes/wprig/languages' '--no-purge'
[07:20:12] Finished 'translate' after 2.48 s
[07:20:12] gulp-imagemin: Minified 0 images
[07:20:12] Finished 'images' after 3.06 s
[07:20:15] Finished 'scripts' after 5.44 s
[07:20:17] Finished 'styles' after 8.01 s
[07:20:17] Starting 'editorStyles'...
[07:20:18] Finished 'editorStyles' after 521 ms
[07:20:30] Finished 'php' after 21 s
[07:20:30] Finished 'buildDev' after 21 s

ataylorme · 2019-07-22T18:09:21Z

gulp/tests/translations/translations.test.js

+ * WP Rig doesn't have any translatable strings in JS.
+ * Once there are some, we can run this test.
+ * failMessage = `The .pot file ${ filePath } does not contain any .js translations`;
+ * expect( fileContents, failMessage ).toContain( '.js' );


@benoitchantre I'm not testing for JS translation in the dev .pot since we don't have any translatable strings yet.

ataylorme · 2019-07-22T18:12:23Z

gulp/tests/prod-build/prod-build.test.js

+
+ // Test that the .pot file contains .js translations
+ failMessage = `The .pot file ${ filePath } does not contain any .js translations`;
+ expect( fileContents, failMessage ).toContain( '.js' );


I am testing JS translations in the prod .pot file since I am mocking the editor-filters.js file from #379

ataylorme · 2019-07-22T18:17:28Z

I think I will try to remove assets/js/src from the exclusion and renaming the JSON files first.

Renaming JSON files to the correct handle name might be tough as I don't know how to decode the hash.

benoitchantre · 2019-07-22T18:36:04Z

Renaming JSON files to the correct handle name might be tough as I don't know how to decode the hash.

I think keeping i18n function names in the minified files is also safer for themes that will be uploaded to wp.org

benoitchantre · 2019-07-22T18:44:08Z

I have been thinking about this and do not know if we should enable all the translation functionality in the default build task. Instead, we can keep translate callable on its own during development and use the option above to toggle it for production.

I’m in favor to remove it from the build task and to keep it callable on its own. It will speed up the build task.

Another argument in that direction is that people who build themes for wp.org don’t need to create a .pot file.

ataylorme · 2019-07-22T19:06:29Z

So generating the .pot file from the production theme compiled files, even when function names aren't minified, doesn't work as the wp-cli i18n make-pot command is hardcoded to ignore *.min.js files.

benoitchantre · 2019-07-22T19:30:18Z

I get many errors from PHP Code Sniffer: Mismatched text domain. Expected 'wp-rig' but got 'prod-theme'.

The error was on my side only: I haven't seen that I had an unwanted subdirectory.
No issue in your script 👍

ataylorme · 2019-07-23T01:51:18Z

@benoitchantre there is some more conversation on github.com/wp-cli/i18n-command/issues/174.

It seems that generating a .pot file from minified JS is an issue, even with keep_fnames set in uglify.

I don't think we can generate the .pot file from the JS source files in the dev theme either as the file names and line numbers for translations will be different from the minified JS.

At this point, I feel out of options. Let me know if you have any suggestions

ataylorme · 2019-07-23T16:48:31Z

@benoitchantre summary of the discussion over on wp-cli is that minified files are not supported for translations.

ideally you would run the string extraction on the source files in assets/js/src, not the minified production files. This way, translators will have the 100% correct context when they try to check the source code to see where a string is used. Looking at minified source code doesn't help there.

I am going to recommend two things:

When translation generation happens JS file minification is disabled
We make translation file generation opt-in instead of opt-out (leaving the default functionality of minifying JS)

I don't think this will be a breaking change as WP Rig currently does not generate translation files (.po, .mo, .json, etc.)

What are your thoughts?

benoitchantre · 2019-07-24T10:43:25Z

@ataylorme I have tested that now with a plugin. It can work under the following conditions:

JS files cannot use .min.js as an extension (prevents wp i18n make-pot to exclude the files, prevents MD5 hash mismatch between unminified and minified files)
JS files need to be unminified before we run wp i18n make-pot
wp i18n make-pot has to exlclude src/ to prevent duplicated strings

If these conditions are met, wp i18n make-json will generate .json files with the correct file names.

For info, I have asked in the WP-CLi Slack channel if this is the recommended way.
Waiting for suggestions from Alain Schlesser or Pascal Birchler.

Another direction is to use --skip-js --merge=languages/wp-rig.pot parameters with wp i18n make-pot and to extract strings with Babel. This can be done with @wordpress/babel-plugin-makepot package. Pascal Birchel has a section in that in his post about Internationalization in WordPress 5.0. There's also a documentation in the Block Editor Handbook. This seems a better solution (no breaking changes).

I'm also in favor to make translation file generation opt-in instead of opt-out for the following reasons:

npm run build will be faster
people who build a theme for wp.org don't need translation files (their are generated when the theme is uploaded/updated)

Update: I just found the following related WP CLI i18n issues (one is from you).

Following your idea, I think that, minification should only be disabled when the theme as internationalized JS files. npm run translate could work only on PHP files. An additional parameter -- js could disable JS minification, generate the .pot file and extract the strings to .json file(s).

ataylorme · 2019-07-27T14:22:17Z

JS files need to be unminified before we run wp i18n make-pot
If these conditions are met, wp i18n make-json will generate .json files with the correct file names.

If the .pot file is generated from unminified files and then the files are minified after will there be an issue because line numbers don't match?

I'm at WP Campus still but I think you and I should schedule some time to discuss this next week.

I've spent many hours on this and we still haven't reached a solution. I think we still need to ask if this is something WP Rig should do or, if like IE11 support, we leave the generation of all the translation files up to the user.

benoitchantre · 2019-07-29T15:31:51Z

I've spent many hours on this and we still haven't reached a solution

Thank you for all your work @ataylorme. Your solution works well with PHP files, and that's already a big improvement from what we have now. Maybe we can move forward with that already and wait for improvements in WP-CLI path handling when generating JSON files.

I'll be on holidays tomorrow evening until August 19, but I'll keep an eye on GitHub and try to schedule some time to discuss this topic with you.

benoitchantre · 2019-08-29T20:58:28Z

@ataylorme As discussed in Slack, here's a summary of the steps for the gulp process. Javascript translation will be handled later (waiting for improvements on wp i18n make-json).

npm run translate

extract strings to a .pot file from the dev theme

npm run bundle

check if there's a .pot file in the dev theme and skip the following steps if no .pot file is found in the dev theme
extract strings to a .pot file from the prod theme
copy existing .po files from dev theme
run string replacements on .po files
generate .mo files from .po files

benoitchantre · 2019-09-30T21:16:01Z

@ataylorme Do you have some time to update this PR?

ataylorme · 2019-10-10T01:19:42Z

Do you have some time to update this PR?

@benoitchantre not currently. I have been very busy with work and personal life recently 😞

Copy .pot, .po, and .mo files to the prod theme

b420679

- Use wp-cli to generate the `.pot` file instead of `gulp-pot`. - Copied translation files include string replacement - Refactor gulp tests and add coverage for translations and prod string replace files.

ataylorme requested review from bamadesigner and mor10 as code owners July 12, 2019 03:31

ataylorme added 6 commits July 11, 2019 20:55

ESLint fixes

92ff15a

Formatting

baec616

Run dev and prod gulp tests concurrently

107897f

Only check slug collision if building for prod

7c209f5

Formatting

34f1b24

Clean up prod tests

02f83e3

ataylorme requested a review from felixarntz as a code owner July 12, 2019 17:08

ESLint cleanup

08c6c14

ataylorme added 2 commits July 17, 2019 16:53

Update translation tasks with some prod tests

3d01177

Remove jest beforeAll and afterAll because they don't work as expected

aad25e7

ataylorme requested a review from benoitchantre July 18, 2019 00:20

ataylorme self-assigned this Jul 18, 2019

ataylorme added bug Something isn't working gulp Gulp related issues needs-testing labels Jul 18, 2019

ataylorme added this to the v2.0.1 milestone Jul 18, 2019

ataylorme added 4 commits July 19, 2019 10:22

Update .mo prod test case

0af925d

Add generateMOfiles task

51a7bc9

test for default strings in prod .mo files

142ceef

Update prod .mo source to prod .po, not dev .po

06c0858

Use WordPress 5.1 .po files for testing

10780b9

- Properly generate `.mo` files with the new gulp task `generateMOfiles` - Test for `.json` files created from `.po` files when doing the production build

ataylorme added 2 commits July 21, 2019 20:40

Update code comment

931c6d6

Correct test description

aaf39f3

Split up translation tasks and add more tests

4067fcf

ataylorme commented Jul 22, 2019

View reviewed changes

This was referenced Jul 22, 2019

i18n make-pot and minified js files wp-cli/i18n-command#140

Closed

Overriding minified JavaScript file exclusion in i18n make-pot wp-cli/i18n-command#174

Closed

ataylorme mentioned this pull request Aug 9, 2019

Add tests for the gulp prod theme build task #579

Merged

7 tasks

felixarntz modified the milestones: v2.0.1, v2.0.2 Sep 3, 2019

Base automatically changed from develop to master December 30, 2020 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translation files added to prod bundle #551

Translation files added to prod bundle #551

ataylorme commented Jul 12, 2019

benoitchantre commented Jul 12, 2019

ataylorme commented Jul 12, 2019

benoitchantre commented Jul 12, 2019

ataylorme commented Jul 18, 2019

ataylorme commented Jul 18, 2019

ataylorme commented Jul 18, 2019

benoitchantre commented Jul 18, 2019

ataylorme commented Jul 20, 2019

benoitchantre commented Jul 20, 2019

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 22, 2019

ataylorme commented Jul 22, 2019

ataylorme Jul 22, 2019

ataylorme Jul 22, 2019

ataylorme commented Jul 22, 2019

benoitchantre commented Jul 22, 2019

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 22, 2019 •

edited

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 23, 2019 •

edited

ataylorme commented Jul 23, 2019

benoitchantre commented Jul 24, 2019 •

edited

ataylorme commented Jul 27, 2019

benoitchantre commented Jul 29, 2019

benoitchantre commented Aug 29, 2019

benoitchantre commented Sep 30, 2019

ataylorme commented Oct 10, 2019

Translation files added to prod bundle #551

Are you sure you want to change the base?

Translation files added to prod bundle #551

Conversation

ataylorme commented Jul 12, 2019

Description

List of changes

Checklist:

benoitchantre commented Jul 12, 2019

ataylorme commented Jul 12, 2019

benoitchantre commented Jul 12, 2019

ataylorme commented Jul 18, 2019

ataylorme commented Jul 18, 2019

ataylorme commented Jul 18, 2019

benoitchantre commented Jul 18, 2019

ataylorme commented Jul 20, 2019

benoitchantre commented Jul 20, 2019

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 22, 2019

ataylorme commented Jul 22, 2019

ataylorme Jul 22, 2019

Choose a reason for hiding this comment

ataylorme Jul 22, 2019

Choose a reason for hiding this comment

ataylorme commented Jul 22, 2019

benoitchantre commented Jul 22, 2019

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 22, 2019 • edited

benoitchantre commented Jul 22, 2019

ataylorme commented Jul 23, 2019 • edited

ataylorme commented Jul 23, 2019

benoitchantre commented Jul 24, 2019 • edited

ataylorme commented Jul 27, 2019

benoitchantre commented Jul 29, 2019

benoitchantre commented Aug 29, 2019

benoitchantre commented Sep 30, 2019

ataylorme commented Oct 10, 2019

ataylorme commented Jul 22, 2019 •

edited

ataylorme commented Jul 23, 2019 •

edited

benoitchantre commented Jul 24, 2019 •

edited