New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n: Replace Jed with Tannin #11493

Merged
merged 7 commits into from Nov 14, 2018

Conversation

@aduth
Member

aduth commented Nov 5, 2018

Related: WordPress/packages#101

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

This comment has been minimized.

Member

omarreiss commented Nov 6, 2018

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

@omarreiss

This comment has been minimized.

Member

omarreiss commented Nov 6, 2018

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?

}
return i18n;
}

This comment has been minimized.

@nerrad

nerrad Nov 6, 2018

Contributor

Do we need a replacement for this?

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

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.

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

This comment has been minimized.

@nerrad

nerrad Nov 6, 2018

Contributor

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?

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

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

This comment has been minimized.

Contributor

nerrad commented Nov 6, 2018

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

This comment has been minimized.

Member

omarreiss commented Nov 6, 2018

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

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 6, 2018

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

This comment has been minimized.

Contributor

nerrad commented Nov 6, 2018

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.

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

This comment has been minimized.

@herregroen

herregroen Nov 6, 2018

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.

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

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.

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

This comment has been minimized.

@herregroen

herregroen Nov 6, 2018

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.

This comment has been minimized.

@swissspidy

swissspidy Nov 6, 2018

Member

Agreed. Jed uses that library as well.

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

Swapped for sprintf-js in 9e07b80

@herregroen

This comment has been minimized.

herregroen commented Nov 6, 2018

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

This comment has been minimized.

Member

omarreiss commented Nov 6, 2018

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.

@swissspidy swissspidy requested a review from ocean90 Nov 6, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 6, 2018

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 😇 ☠️

@aduth aduth force-pushed the try/tannin branch from 9e07b80 to 8f74310 Nov 9, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 9, 2018

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

This comment has been minimized.

Member

aduth commented Nov 14, 2018

I'd appreciate a review on this one.

@nerrad

nerrad approved these changes Nov 14, 2018

@aduth aduth merged commit 27b1c23 into master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the try/tannin branch Nov 14, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

Thanks @nerrad

@nerrad

This comment has been minimized.

Contributor

nerrad commented Nov 14, 2018

Don't forget the CHANGELOG.md entry.

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

Don't forget the CHANGELOG.md entry.

Good call 😄 See #11867

@ocean90 ocean90 added this to the 4.4 milestone Nov 14, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

i18n: Replace Jed with Tannin (WordPress#11493)
* i18n: Subtitute Tannin for Jed

* i18n: Improve bad practice

Placeholders should be consistent across all variations

* i18n: Support omitted plural forms expression

* i18n: Align locale data default to `gutenberg_get_jed_locale_data`

* i18n: Swap tannin/sprintf with sprintf-js

* i18n: Guard against thrown sprintf errors

* i18n: Restore memize identity mock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment