search

Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 798 forks source link

Blocks: i18n: Translatable strings from *.min.js #21343

Closed yuliyan closed 2 years ago

yuliyan commented 3 years ago

Impacted plugin

Jetpack

Steps to Reproduce

  1. Download Jetpack from https://wordpress.org/plugins/jetpack/
  2. Run wp i18n make-pot .
  3. Inspect the generated POT file to confirm translatable strings from *.min.js files are missing.
  4. Remove the .min suffix from the filename of any of the JS files that contain translatable strings (e.g. _inc/blocks/editor.min.js) and run wp i18n make-pot . again.
  5. Inspect the generated POT file and confirm the strings from that file are now included in the POT.

A clear and concise description of what you expected to happen.

AFAIK, the plugin directory is using the same (or pretty similar) approach to wp i18n make-pot . to generate POT file for a plugin and import into its translate.wordpress.org project. By default, the make-pot command excludes some files and directories from scanning, including *.min.js files. As a result:

Other information

.min suffix introduced in: https://github.com/Automattic/jetpack/commit/be80d269a407518138060e8417c522e051d58b9a Related issue: https://github.com/Automattic/jetpack/issues/21204

jeherve commented 3 years ago

This also came up in:

kraftbj commented 3 years ago

Noting that we switched to the .min.js file extension to prevent the built files from being handled by wp.com's JS minimization script, which started breaking when we dropped IE 11 (thus output into more modern JS).

kraftbj commented 3 years ago

This is apparently expected behavior for the make-pot CLI command:

[--exclude=] [...] The following files and folders are always excluded: node_modules, .git, .svn, .CVS, .hg, vendor, *.min.js.

from https://developer.wordpress.org/cli/commands/i18n/make-pot/

I have in my mind that we checked GlotPress and confirmed the strings were there? If so, make-pot isn't the issue (I wouldn't think). If not, then we need to figure out something—either via make-pot or Systems re the wp.com need for .min.js that pushed us this way.

yuliyan commented 3 years ago

@kraftbj, I was able to get make-pot pick up the strings from the JS files locally by simply renaming the file from editor.min.js to editor.js, which makes me think that the min suffix is what's causing it.

Previously, there was a problem with the translation function names not being preserved in the minified output, but that got resolved in https://github.com/Automattic/jetpack/commit/1e56ff33dae8ef84c23d8621b30f6e974f32ab2a and by inspecting the plugin files I can confirm that __ function names are indeed preserved in the output files.

I'm not familiar with the reason why .min.js is needed on WP.com, but if that's the case, a possible workaround could be to have two copies of the same JS files with different filenames in the release, i.e. to have both editor.min.js and editor.js.

kraftbj commented 3 years ago

I'm not familiar with the reason why .min.js is needed on WP.com

WordPress.com has an automatically minimizing process for JS upon being served to a client. That process assumes ES5 code. When we dropped IE 11 support, we started shipping ES6 code which WordPress.com's minimizing function chokes on (thus breaking blocks on WordPress.com with the same code committed there as in Jetpack).

Systems said that we should update the file name to min.js, which the server ignores for that script.

Duplication is an okay immediate solution to get translations back for customers that we have control over, but I don't like it as a long-term solution as it doubles the JS payload we're shipping to customers with no benefit (beyond hacking around ecosystem issues).

tl;dr duplicate files is okay immediately but I'll task my team (or myself) to investigate further the make-pot or Systems routes towards a different solution.

anomiex commented 3 years ago

but I don't like it as a long-term solution as it doubles the JS payload we're shipping to customers with no benefit (beyond hacking around ecosystem issues).

"Shipping to customers" as in including both the minified and non-minifed built JS files in the Jetpack plugin? Or "shipping to customers" thinking that visitors to the site would get served both copies? To be clear, the latter would not happen.

kraftbj commented 3 years ago

Confirm, the latter wouldn't happen. I'd not want to ship too much extra to sites in the plugin upgrade routine.

samiff commented 3 years ago

Related: https://github.com/Automattic/jetpack/pull/21367

yuliyan commented 3 years ago

Do you have any update on this issue?

jeherve commented 3 years ago

@yuliyan We've been discussing potential solutions here: p9dueE-3Gb-p2

We'd be happy to have you chime in on that discussion!

anomiex commented 2 years ago

I think that #21748 fixed this entirely after all. As far as I can tell none of the files that were being processed by gulp use @wordpress/i18n (and now we've switched them to being processed by Webpack anyway).