WordPress i18n package: Initial commit #96

youknowriad · 2018-03-22T12:35:14Z

This pull request seeks to add a new package @wordpress/i18n package extracted from Gutenberg.

It includes the JavaScript library used to translate the strings
A babel plugin and node script needed to make sure your plugin is i18n ready in the WordPress.org repository (these two scripts could be removed later if the repository implements JavaScript strings discovery).

codecov · 2018-03-22T12:38:35Z

Codecov Report

Merging #96 into master will decrease coverage by 6.88%.
The diff coverage is 41.17%.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
- Coverage   71.42%   64.54%   -6.89%     
==========================================
  Files          38       41       +3     
  Lines         462      598     +136     
  Branches       87      118      +31     
==========================================
+ Hits          330      386      +56     
- Misses        112      170      +58     
- Partials       20       42      +22

Impacted Files	Coverage Δ
packages/i18n/tools/pot-to-php.js	`0% <0%> (ø)`
packages/babel-plugin-makepot/src/index.js	`39.58% <39.58%> (ø)`
packages/i18n/src/index.js	`94.73% <94.73%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0004c6a...7c270d8. Read the comment docs.

adamsilverstein · 2018-03-22T13:55:41Z

@youknowriad thank you for opening this PR! I see codedev complaining about reduced tests, can we add more tests to increase coverage?

youknowriad · 2018-03-22T14:00:17Z

@adamsilverstein I think it's pretty well tested, what's not tested is the pot-2-php script which is just a temporary script to generate a dummy php file (for discovery in .org). I think this file is hurting the coverage but I don't think it's worth testing.

Edit: I see some small functions are not tested, I'll add tests for those.

youknowriad · 2018-03-22T17:06:04Z

I added unit tests, the coverage is still decreasing because of pot-to-php script but as I said, this one is not worth testing.

ocean90 · 2018-03-26T10:58:42Z

The usage of pot-to-php should not be promoted like this, because using one file for all strings with get_translations_for_domain() is not really a solution. I suggest to not include it in this package.

String extraction to translate.w.org can be added quite easily once we have identified how and which translations should get extracted. See https://core.trac.wordpress.org/ticket/20491#comment:82.

youknowriad · 2018-03-26T11:36:58Z

String extraction to translate.w.org can be added quite easily once we have identified how and which translations should get extracted. See https://core.trac.wordpress.org/ticket/20491#comment:82.

That would be great, I'm not aware of the work being done to do so but I believe this PR identifies properly what should be extracted (all the APIs documented in the README). Who can help with this?

I'd be happy to remove the pot-to-php once done but right now we need a solution for it. I believe the babel plugin is useless as well once the extraction lands.

ocean90 · 2018-03-26T12:19:22Z

but right now we need a solution for it.

With "we" you mean Gutenberg? Don't see why that script can't remain there as a standalone script. Since the package should only be published once core and w.org fully supports this proposal for the JS i18n API I don't see the need for the script.

I believe the babel plugin is useless as well once the extraction lands.

The babel plugin doesn't seem useless since that's what w.org is probably going to use for the extraction. Same for authors that don't host their plugins/themes on w.org.

The current extraction for PHP can be found here: https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/plugin-directory/cli/i18n/class-code-import.php
So for the JS strings we need something that can be run similar to this.

I believe this PR identifies properly what should be extracted

How does it handle a JS file with multiple text domains? Can a fixed text domain be passed to the script? Can it handle const __ = (text, domain = 'gutenberg') => {...} as proposed in #4147? Does it work on build files?

youknowriad · 2018-03-26T12:25:08Z

With "we" you mean Gutenberg?

I was thinking "we" as Plugin authors, Core developpers, if we land this package as a generic package we need a way to make these strings translatable. If not, the package is useless.

How does it handle a JS file with multiple text domains? Can a fixed text domain be passed to the script? Can it handle const __ = (text, domain = 'gutenberg') => {...} as proposed in #4147?

Yes, it does the domain is the last arg for all these functions.

Does it work on build files,

That's a good point and I don't really know. Right now we use it on unminified files only. The problem with minification... is that we don't control the tool doing it, it'd be hard to come up with a solution that works consistently across tools. Thoughts @aduth

aduth · 2018-03-26T13:48:36Z

The Babel plugin itself doesn't make a distinction on domain. It will extract the strings, regardless of the domain passed (if any), but there is only a single output file, so a source file with two separate domains would still be processed into a single .pot file.

Minification could be an issue with "mangling", where minifiers choose a shorter auto-generated variable name to use in place of the original name. Since the plugin relies on the names of the functions (__, etc), these strings would not be sourced. At least for UglifyJS, there are options to exempt certain reserved variables to avoid minification:

https://github.com/mishoo/UglifyJS2#cli-mangle-options

I expect this may be most problematic for trying to extract strings from the plugin repository. During a plugin's own build process, this is unlikely to occur, since the Babel plugin would apply its transforms before minification is applied.

aduth · 2018-03-27T19:48:29Z

packages/i18n/tools/pot-to-php.js

@@ -0,0 +1,135 @@
+#!/usr/bin/env node


Did you run this file through some formatter? It's got all sorts of styling issues which aren't present in the same file from the Gutenberg repository (spaces instead of tabs, double quotes instead of single quotes).

Probably my default prettier setup :P I'll fix it. Btw, we need eslint support :)

aduth · 2018-03-27T19:51:34Z

packages/babel-plugin-i18n/README.md

@@ -0,0 +1,18 @@
+i18n


Re: Naming. I think the title here should reflect the fact that it's a Babel plugin. Maybe at least "i18n Babel Plugin".

But aside from it being scoped to WordPress, and WordPress using gettext for i18n, I feel a bit odd calling this a "i18n Babel Plugin", since gettext is just one method of internationalizing.

The equivalent PHP tool is called makepot.php, so maybe babel-plugin-makepot if we want consistency (whether or not the name on its own is great).

Otherwise some variations of words in babel-plugin-extract-gettext-strings ? -extract-strings? -extract-gettext ? -gettext-strings ?

babel-plugin-makepot sounds good to me

youknowriad · 2018-03-28T08:53:10Z

Feedback addressed, this should be in a better shape.

aduth

Couple suggestions, but looking to be in good shape.

aduth · 2018-04-03T01:13:55Z

packages/babel-plugin-makepot/package.json

+	"bugs": {
+		"url": "https://github.com/WordPress/packages/issues"
+	},
+	"main": "src/index.js",


The root build configuration is such that a build distributable will be created for this, even if the src/index.js doesn't require transpilation. We should consider one of:

Target build/index.js as main, even if not strictly necessary, merely for consistency with other packages

Introduce a pattern for excluding particular projects from the Babel build step

I'm using the build folders right now, let's add this option separately because it seems useful for other packages as well.

aduth · 2018-04-03T01:15:11Z

packages/i18n/README.md

+
+You can use the [WordPress i18n babel plugin](../babel-plugin-makepot/README.md) to generate a `.pot` file containing all your localized strings.
+
+The package also includes a `pot-to-php` script used to generate a php files containing the messages listed in a `.pot` file. This is usefull to trick WordPress.org translation strings discovery since at the moment, WordPress.org is not capable of parsing strings directly from JavaScript files.


Typo: "usefull" -> "useful"

aduth · 2018-04-03T01:17:11Z

packages/i18n/README.md

+The package also includes a `pot-to-php` script used to generate a php files containing the messages listed in a `.pot` file. This is usefull to trick WordPress.org translation strings discovery since at the moment, WordPress.org is not capable of parsing strings directly from JavaScript files.
+
+```sh
+node tools/pot-to-php languages/myplugin.pot languages/myplugin-translations.php text-domain


I think we want to make this a proper bin entry in package.json:

https://docs.npmjs.com/files/package.json#bin

That way, a developer could execute via npx pot-to-php.

As-is, the code snippet isn't very usable (would need to at least be prefixed with ./node_modules/@wordpress/i18n).

aduth · 2018-04-03T01:18:23Z

packages/i18n/README.md

+
+See: http://www.diveintojavascript.com/projects/javascript-sprintf
+
+`setLocaleData( data: Object, domain: string )`


Thanks for fixing the documentation I keep neglecting to fix 😄

youknowriad · 2018-04-04T08:54:07Z

From yesterday's chat, there's no blocking here. So moving forward.

youknowriad self-assigned this Mar 22, 2018

youknowriad requested a review from aduth March 22, 2018 12:35

WordPress i18n package: Initial commit

368b567

youknowriad force-pushed the add/i18n branch from ec9d4b4 to 368b567 Compare March 22, 2018 12:36

i18n: Adding unit tests for the different translation functions

576230a

i18n: Move the babel plugin to a separate package

9fa7e0c

aduth reviewed Mar 27, 2018

View reviewed changes

Reset code style of pot-to-php.js

c974b5d

youknowriad force-pushed the add/i18n branch from 305283d to c974b5d Compare March 28, 2018 08:50

Rename the i18n babel plugin

20466f5

aduth reviewed Apr 3, 2018

View reviewed changes

youknowriad added 3 commits April 3, 2018 08:19

i18n: Fix typo in documentation

bb88e30

Making pot-to-php an npm binary

00a36a1

using the built folders

7c270d8

youknowriad merged commit ec90828 into master Apr 4, 2018

youknowriad deleted the add/i18n branch April 4, 2018 08:54

ocean90 mentioned this pull request Apr 4, 2018

i18n: Use wp-components script handle to pass locale data to wp.i18n WordPress/gutenberg#5910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WordPress i18n package: Initial commit #96

WordPress i18n package: Initial commit #96

youknowriad commented Mar 22, 2018

codecov bot commented Mar 22, 2018 •

edited

adamsilverstein commented Mar 22, 2018

youknowriad commented Mar 22, 2018 •

edited

youknowriad commented Mar 22, 2018

ocean90 commented Mar 26, 2018

youknowriad commented Mar 26, 2018

ocean90 commented Mar 26, 2018

youknowriad commented Mar 26, 2018 •

edited

aduth commented Mar 26, 2018 •

edited

aduth Mar 27, 2018

youknowriad Mar 27, 2018 •

edited

aduth Mar 27, 2018

youknowriad Mar 27, 2018

youknowriad commented Mar 28, 2018

aduth left a comment

aduth Apr 3, 2018

youknowriad Apr 3, 2018

aduth Apr 3, 2018

aduth Apr 3, 2018

aduth Apr 3, 2018

youknowriad commented Apr 4, 2018


		You can use the [WordPress i18n babel plugin](../babel-plugin-makepot/README.md) to generate a `.pot` file containing all your localized strings.

		The package also includes a `pot-to-php` script used to generate a php files containing the messages listed in a `.pot` file. This is usefull to trick WordPress.org translation strings discovery since at the moment, WordPress.org is not capable of parsing strings directly from JavaScript files.


		See: http://www.diveintojavascript.com/projects/javascript-sprintf

		`setLocaleData( data: Object, domain: string )`

WordPress i18n package: Initial commit #96

WordPress i18n package: Initial commit #96

Conversation

youknowriad commented Mar 22, 2018

codecov bot commented Mar 22, 2018 • edited

Codecov Report

adamsilverstein commented Mar 22, 2018

youknowriad commented Mar 22, 2018 • edited

youknowriad commented Mar 22, 2018

ocean90 commented Mar 26, 2018

youknowriad commented Mar 26, 2018

ocean90 commented Mar 26, 2018

youknowriad commented Mar 26, 2018 • edited

aduth commented Mar 26, 2018 • edited

Choose a reason for hiding this comment

youknowriad Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 28, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 4, 2018

codecov bot commented Mar 22, 2018 •

edited

youknowriad commented Mar 22, 2018 •

edited

youknowriad commented Mar 26, 2018 •

edited

aduth commented Mar 26, 2018 •

edited

youknowriad Mar 27, 2018 •

edited