i18n: Replace Jed with Tannin #11493

aduth · 2018-11-05T14:55:08Z

This pull request seeks to propose an internal refactoring of the @wordpress/i18n module to replace Jed with the largely-compatible Tannin library. This has no impact on the public interface of the module, and instead serves as an overall performance, memory use, and bundle size improvement (see benchmarks).

Context: Summarized at WordPress/packages#101, Jed's implementation has a critical performance flaw in that it parses and compiles the locale plural forms expression every time a translation function is called with pluralization or when locale data is not explicitly provided (e.g. default English). While memoization remedies the issue, it's more a bandaid solution in that (a) the underlying operation is still expensive† and (b) it incurs an additional cost of memory storage of redundant cached data. Tannin addresses these issues by computing the parsed evaluator of plural forms at most a single time, and by having more efficient expression parsing and evaluation to begin with.

† Even with caching, the current Gutenberg master incurs a plural forms parse nearly 300 times during the initial load.

Testing Instructions:

There should be no impact on the application. There are numerous layers of unit testing to validate variations of translations, but verify that the correct strings are shown in general usage, notably:

Non-English locales
Pluralization

Ensure unit tests pass:

npm run test-unit

omarreiss · 2018-11-06T13:48:14Z

I have no objections. Would like @herregroen to sign off on this being parsable by https://github.com/wp-cli/i18n-command/

omarreiss · 2018-11-06T13:48:56Z

Also, if we end up using this, I'd suggest bringing this into Gutenberg and publishing as @wordpress/gettext package.

nerrad

Looks good, and nice job humbly hiding the fact you wrote Tannin ;)

The only minor concern I have here is that there seems to be a decrease in useful dev feedback when something is wrong. Intentional?

nerrad · 2018-11-06T13:37:27Z

packages/i18n/src/index.js

-	}
-
-	return i18n;
-}


Do we need a replacement for this?

Do we need a replacement for this?

No, I don't think so. It's not exposed as a public API, and its previous purpose of in-time initialization is now served instead by combination of initialize-at-assignment and preparing domain data within dcnpgettext.

nerrad · 2018-11-06T13:42:12Z

packages/i18n/src/index.js

 	}
-} );
+
+	return i18n.dcnpgettext( domain, context, single, plural, number );


so I'm guessing this is to prevent errors when locale data for the domain has not been set yet? I'm concerned about the potential for silent fails here. Should we at least throw a development warning (a-la invariant or equivalent) when this is needed?

so I'm guessing this is to prevent errors when locale data for the domain has not been set yet? I'm concerned about the potential for silent fails here. Should we at least throw a development warning (a-la invariant or equivalent) when this is needed?

Related: #11493 (comment)

I'll look into what the error paths would be and consider whether they're likely to be encountered and if there's a way to preempt / capture them.

nerrad · 2018-11-06T13:52:17Z

Also, if we end up using this, I'd suggest bringing this into Gutenberg and publishing as @wordpress/gettext package.

Do you mean publishing Tannin as @wordpress/gettext?

omarreiss · 2018-11-06T13:55:08Z

@nerrad yes. If we are going to depend on this, we might as well transfer ownership to the WordPress community right?

youknowriad · 2018-11-06T13:58:24Z

I think it's fine to keep it separate. Andrew worked on this as a personal project and we can use it like we use any other personal project. We already use other personal packages from Andrew and Me and other people outside of the community. While I'd be fine giving my projects to WordPress, I think it doesn't make sense to tell people: if you want to us use this project, move it to WordPress.

nerrad · 2018-11-06T13:59:00Z

I don't think we have to in order to use it, but it certainly is early enough in its creation to encourage doing so.

herregroen · 2018-11-06T13:56:23Z

packages/i18n/src/index.js

-	try {
-		return getI18n().dcnpgettext( domain, context, single, plural, number );
-	} catch ( error ) {
-		logErrorOnce( 'Jed localization error: \n\n' + error.toString() );


This would make wp.i18n silent on errors if I'm reading everything correctly.

logErrorOnce should be passed to the onMissingKey option from Tannin in my opinion.

The previous try / catch existed largely because Jed is rather throw-happy with how it deals with unexpected inputs. Though it's similarly fair that Tannin, called incorrectly, could also throw (as unhandled). It's maybe curious to think on how much it could make sense to validate inputs from the @wordpress/i18n.

I'm not so sure about providing the missing key callback, only because for the default English locale, we don't provide any initialization data, meaning we rely on the missing key behavior intentionally as the default behavior.

herregroen · 2018-11-06T14:00:48Z

packages/i18n/src/index.js


-let i18n;
+export { default as sprintf } from '@tannin/sprintf';


I'm wondering if it wouldn't be preferable to simply use something like https://github.com/alexei/sprintf.js instead of introducing our own new library for this. sprintf seems like something that would already have a well-test well-maintained library.

Agreed. Jed uses that library as well.

It's an easy swap. I'd assumed sprintf-js to be quite bloated in its full-featuredness, though in truth it's fairly reasonable and efficient. It's still 6x the size of @tannin/sprintf (3x gzipped), though also includes functionality not supported by sprintf -- albeit the lesser-used functionality of printf patterns.

As far as implementation and performance, it holds an internal cache for format strings, which could be argued as a minor memory leak. Without caching, they perform similarly.

Maturity is a fair point to argue. In all, it's a fair swap I think.

Swapped for sprintf-js in 9e07b80

herregroen · 2018-11-06T14:02:18Z

I have no objections. Would like @herregroen to sign off on this being parsable by https://github.com/wp-cli/i18n-command/

Should be no issues here.

omarreiss · 2018-11-06T14:03:33Z

We already use other personal packages from Andrew and Me and other people outside of the community.

Although I don't understand why you'd want to keep those personal, I am fine either way.

aduth · 2018-11-06T16:41:44Z

Although I don't understand why you'd want to keep those personal, I am fine either way.

Without belaboring the point, it was a hobby / passion project intentionally developed outside regular constraints / commitments. While I don't have a strong resistance to transferring ownership, I also don't see that it be strictly necessary, nor think there should be an expectation that all hobby projects of any WordPress contributor be brought under the umbrella of the organization.

And as with any permissibly-licensed open-source project, it's entirely forkable should I at any point go rogue 😇 ☠️

Placeholders should be consistent across all variations

aduth · 2018-11-09T21:27:02Z

I've pushed a rebased copy of the branch.

Regarding error handling, I've restored the try / catch which had existed for sprintf, since sprintf-js does throw errors (example).

I'd actually reinstated it to dcnpgettext locally, but I'd found myself completely unable to come up with a functioning unit test covering the occasion where an error would be thrown, which led me to the conclusion that it's not necessary (and thus I've not included it here).

aduth · 2018-11-14T13:29:52Z

I'd appreciate a review on this one.

aduth · 2018-11-14T14:51:32Z

Thanks @nerrad

nerrad · 2018-11-14T15:03:40Z

Don't forget the CHANGELOG.md entry.

aduth · 2018-11-14T16:01:03Z

Don't forget the CHANGELOG.md entry.

Good call 😄 See #11867

aduth added Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality [Package] i18n /packages/i18n labels Nov 5, 2018

aduth requested review from swissspidy, nerrad and omarreiss November 5, 2018 17:54

nerrad reviewed Nov 6, 2018

View reviewed changes

herregroen reviewed Nov 6, 2018

View reviewed changes

swissspidy requested a review from ocean90 November 6, 2018 14:38

aduth added 6 commits November 9, 2018 16:22

i18n: Subtitute Tannin for Jed

d3d86b0

i18n: Improve bad practice

9ff9817

Placeholders should be consistent across all variations

i18n: Support omitted plural forms expression

70b7544

i18n: Align locale data default to gutenberg_get_jed_locale_data

ae1012c

i18n: Swap tannin/sprintf with sprintf-js

1c76f03

i18n: Guard against thrown sprintf errors

8f74310

aduth force-pushed the try/tannin branch from 9e07b80 to 8f74310 Compare November 9, 2018 21:23

i18n: Restore memize identity mock

9b8f5b0

nerrad approved these changes Nov 14, 2018

View reviewed changes

aduth merged commit 27b1c23 into master Nov 14, 2018

aduth deleted the try/tannin branch November 14, 2018 14:50

aduth mentioned this pull request Nov 14, 2018

i18n: Add CHANGELOG entry for Tannin refactor #11867

Merged

ocean90 added this to the 4.4 milestone Nov 14, 2018

jsnajdr mentioned this pull request Feb 13, 2019

i18n: Prepare package for Calypso Automattic/wp-calypso#29229

Merged

jsnajdr mentioned this pull request Feb 25, 2022

Avoid sending empty script translations WordPress/wordpress-develop#2354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: Replace Jed with Tannin #11493

i18n: Replace Jed with Tannin #11493

aduth commented Nov 5, 2018 •

edited

Loading

omarreiss commented Nov 6, 2018

omarreiss commented Nov 6, 2018

nerrad left a comment

nerrad Nov 6, 2018

aduth Nov 6, 2018 •

edited

Loading

nerrad Nov 6, 2018

aduth Nov 6, 2018

nerrad commented Nov 6, 2018

omarreiss commented Nov 6, 2018

youknowriad commented Nov 6, 2018

nerrad commented Nov 6, 2018

herregroen Nov 6, 2018

aduth Nov 6, 2018 •

edited

Loading

herregroen Nov 6, 2018

swissspidy Nov 6, 2018

aduth Nov 6, 2018

aduth Nov 6, 2018

herregroen commented Nov 6, 2018

omarreiss commented Nov 6, 2018

aduth commented Nov 6, 2018

aduth commented Nov 9, 2018

aduth commented Nov 14, 2018

aduth commented Nov 14, 2018

nerrad commented Nov 14, 2018

aduth commented Nov 14, 2018


		let i18n;
		export { default as sprintf } from '@tannin/sprintf';

i18n: Replace Jed with Tannin #11493

i18n: Replace Jed with Tannin #11493

Conversation

aduth commented Nov 5, 2018 • edited Loading

omarreiss commented Nov 6, 2018

omarreiss commented Nov 6, 2018

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented Nov 6, 2018

omarreiss commented Nov 6, 2018

youknowriad commented Nov 6, 2018

nerrad commented Nov 6, 2018

Choose a reason for hiding this comment

aduth Nov 6, 2018 • edited Loading

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

herregroen commented Nov 6, 2018

omarreiss commented Nov 6, 2018

aduth commented Nov 6, 2018

aduth commented Nov 9, 2018

aduth commented Nov 14, 2018

aduth commented Nov 14, 2018

nerrad commented Nov 14, 2018

aduth commented Nov 14, 2018

aduth commented Nov 5, 2018 •

edited

Loading

aduth Nov 6, 2018 •

edited

Loading

aduth Nov 6, 2018 •

edited

Loading