Include strings from Jetpack in bundle #2617

mchowning · 2020-09-14T21:00:17Z

Updating our bundle scripts to ensure that the strings from the jetpack/ repo are included. We are first retrieving the strings from the jetpack repo in a jetpack-$PLATFORM.pot file. I am then merging that into the gutenberg-$PLATFORM.pot file along with the strings from gutenberg. That gutenberg-$PLATFORM.pot file is then passed to the relevant ./bin/po2XXXX.js script.

Note that I created this branch off of trunk in order to avoid having additional string changes on account of the updates to the submodules on develop. I anticipate that before merging this PR I'll want to rebase this PR to move the package.json changes on top of the appropriate branch (probably develop) and drop the strings changes I have committed in 68a00fd.

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.

mchowning · 2020-09-14T21:03:46Z

package.json

@@ -66,15 +66,14 @@
    "install:wpcli": "(test -x bin/wp-cli.phar || curl -Ls https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli.phar -o bin/wp-cli.phar && chmod +x bin/wp-cli.phar) && bin/wp-cli.phar --info",
    "prewp": "npm run install:wpcli",
    "wp": "php -d memory_limit=4G bin/wp-cli.phar",
-    "makepot:android": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.android.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain gutenberg-android.pot",
-    "makepot:ios": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.ios.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain gutenberg-ios.pot",
+    "makepot": "npm run wp -- i18n make-pot ./jetpack/extensions --include=*.native.js,*.$PLATFORM.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain jetpack-$PLATFORM.pot && npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.$PLATFORM.js --exclude=test/* --subtract=./gutenberg.pot --merge=./jetpack-$PLATFORM.pot --ignore-domain gutenberg-$PLATFORM.pot ",
    "premakepot:gutenberg": "npm run clean:gutenberg && npm run build:gutenberg",
    "makepot:gutenberg": "npm run wp -- i18n make-pot ./gutenberg/build --ignore-domain gutenberg.pot",
    "postmakepot:gutenberg": "npm run clean:gutenberg",
    "pregenstrings": "test -f gutenberg.pot || npm run makepot:gutenberg",


I am not sure if there's anything similar we need to do for Jetpack like we need to do for gutenberg. The Jetpack submodule we have checked out has a languages/ folder, that includes, among other things, jetpack.pot file. That jetpack.pot file does not, however, appear to have all the strings we need since it is missing the "Link address to Google Maps" string (I'm guessing because it is in a native.js file). In addition, that entire languages/ folder has been removed on master.

Basically, there is more work to be done here to get a better understanding of the jetpack build process and either (a) verify that we do not need to do anything more to get all the relevant jetpack strings; or (b) run any necessary jetpack builds as part of the bundling process.

Tug

Code looks good. One problem with this approach though is that, as you mentioned, we don't (yet) include the strings from the jetpack project, meaning that we won't have all the strings from jetpack (only those from .native.js and .platform.js files).
Moreover, including all the strings from jetpack might be overkill as it's a much larger project which has many strings.

I tried a different approach here though it has its drawbacks (the fact that we lose translator comments)

Tug · 2020-09-16T11:24:32Z

package.json

@@ -66,15 +66,14 @@
    "install:wpcli": "(test -x bin/wp-cli.phar || curl -Ls https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli.phar -o bin/wp-cli.phar && chmod +x bin/wp-cli.phar) && bin/wp-cli.phar --info",
    "prewp": "npm run install:wpcli",
    "wp": "php -d memory_limit=4G bin/wp-cli.phar",
-    "makepot:android": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.android.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain gutenberg-android.pot",
-    "makepot:ios": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.ios.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain gutenberg-ios.pot",
+    "makepot": "npm run wp -- i18n make-pot ./jetpack/extensions --include=*.native.js,*.$PLATFORM.js --exclude=test/* --subtract=./gutenberg.pot --ignore-domain jetpack-$PLATFORM.pot && npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.$PLATFORM.js --exclude=test/* --subtract=./gutenberg.pot --merge=./jetpack-$PLATFORM.pot --ignore-domain gutenberg-$PLATFORM.pot ",


Do you think we could reduce the amount of pot files generated by having something like:

i18n make-pot . --include=./jetpack/extensions/**/*.native.js,./jetpack/extensions/**/*.$PLATFORM.js,./gutenberg/packages/**/*.native.js,./gutenberg/packages/**/*.$PLATFORM.js

?

mchowning · 2020-09-18T18:08:22Z

Thanks for reviewing @Tug!

Do you think we could reduce the amount of pot files generated by having something like

Makes sense. I can try that.

we won't have all the strings from jetpack (only those from .native.js and .platform.js files).

I see what you mean. Definitely need to fix that if we use this approach.

including all the strings from jetpack might be overkill as it's a much larger project which has many strings.

I agree, this seems like a pretty big concern.

I tried a different approach here though it has its drawbacks (the fact that we lose translator comments)

I like the approach in your PR better. It seems much more robust. Like you said, it does seem like we need to figure out the translator's comments issue first. I'm inclined to put more work toward fixing up your approach over the approach in this PR.

cameronvoell · 2020-09-28T03:55:52Z

@mchowning Just reminding that we'll be cutting the 1.38 release branch later in the day Monday 9/28. Please try to get this merged by then or bump the milestone. Thanks much 🙇

mchowning · 2022-01-18T19:14:59Z

Going ahead and closing this since our translations pipeline has been updated to address this in a different way.

mchowning added 2 commits September 14, 2020 16:40

Update makepot script to include strings from jetpack/

2401f3c

Updated with strings from jetpack/

68a00fd

mchowning added the release-process label Sep 14, 2020

mchowning added this to the 1.38 milestone Sep 14, 2020

mchowning requested review from Tug and cameronvoell September 14, 2020 21:00

mchowning commented Sep 14, 2020

View reviewed changes

Tug mentioned this pull request Sep 16, 2020

Generate translation files from bundles instead of native sources #2622

Closed

4 tasks

Tug reviewed Sep 16, 2020

View reviewed changes

mchowning modified the milestones: 1.38, 1.39 Sep 28, 2020

mchowning closed this Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include strings from Jetpack in bundle #2617

Include strings from Jetpack in bundle #2617

mchowning commented Sep 14, 2020 •

edited

mchowning Sep 14, 2020 •

edited

Tug left a comment •

edited

Tug Sep 16, 2020

mchowning commented Sep 18, 2020

cameronvoell commented Sep 28, 2020

mchowning commented Jan 18, 2022

Include strings from Jetpack in bundle #2617

Include strings from Jetpack in bundle #2617

Conversation

mchowning commented Sep 14, 2020 • edited

mchowning Sep 14, 2020 • edited

Choose a reason for hiding this comment

Tug left a comment • edited

Choose a reason for hiding this comment

Tug Sep 16, 2020

Choose a reason for hiding this comment

mchowning commented Sep 18, 2020

cameronvoell commented Sep 28, 2020

mchowning commented Jan 18, 2022

mchowning commented Sep 14, 2020 •

edited

mchowning Sep 14, 2020 •

edited

Tug left a comment •

edited