Gutenberg: Load Jetpack block translations in Calypso synchronously #28304

tyxla · 2018-11-06T12:09:29Z

We already have a working i18n system for Jetpack blocks that works well in wp-admin of Jetpack sites. However, currently, we don't serve those translations for Jetpack blocks to Calypso. This means that any of the Jetpack blocks aren't being localized.

This isn't straightforward, because those translations are managed at WP.org, and Calypso's translations are served by WP.com. So what we're already doing on the WP.com side is:

Indexing the localized strings in WP.com
Fetching and storing all Jetpack block translations from all locales.
Serving the translations for Jetpack blocks separately in WP.com

So, currently we'll have the translations available at URLs like: https://widgets.wp.com/languages/jetpack-gutenberg-blocks/bg.json

Next we need to actually pass these to Calypso, so blocks can be translated.

However, it's worth to mention that asynchronously loading those translations has some challenges we don't have solutions for:

Block registration occurs early. If it occurs before translations are loaded, we'll end up with untranslated block names, descriptions and everything else that's not rendered live.
Blocks can't be registered late. If we try registering them late, Gutenberg might think these blocks don't exist, and will throw some warnings, and we don't want that.

We can register blocks without translations, then after translations are loaded, unregister and register them again with translations, however theoretically this will break those blocks in cases where they're needed while we're doing this re-registration. Even though we'll need to come up with a totally different mechanism for registering blocks, this is an alternative approach that is probably worth experimenting with.

So, this PR suggests that we load the translations synchronously, and we instantiate the editor after that. This way we'll only have to register the blocks once, and at that time they'll actually be translated already. The downside with this approach is that the first time translations are loaded, it might cause a little delay before initializing the editor. Any further initialization should be quick, as the translations file will already be available in the browser cache.

Changes proposed in this Pull Request

Synchronously load translations for Jetpack blocks.

Testing instructions

Checkout this branch.
Spin up Calypso: npm start.
Select Bulgarian as your profile language in http://calypso.localhost:3000/me/account
Load http://calypso.localhost:3000/gutenberg/post/:site where :site: is one of your sites.
Start writing a post.
Verify the Jetpack blocks (Related Posts, Markdown, Tiled Gallery) and Publicize extension are translated.
Select English as your profile language in http://calypso.localhost:3000/me/account
Load http://calypso.localhost:3000/gutenberg/post/:site where :site: is one of your sites.
Start writing a post.
Verify no requests to https://widgets.wp.com/languages/jetpack-gutenberg-blocks/* are made in attempt to load i18n files for blocks.

Note

This is currently blocked by #28867.

matticbot · 2018-11-06T12:09:31Z

Test live: https://calypso.live/?branch=try/gutenberg-calypso-jetpack-i18n-take-1

vindl · 2018-11-06T21:22:15Z

@tyxla just a note that I've recently refactored the way the editor is initialized in order to resolve some bugs, which might have some consequences for your approach here: #28302

vindl · 2018-11-06T21:24:39Z

client/gutenberg/editor/controller.js

@@ -42,6 +48,36 @@ function registerDataPlugins( userId ) {
 	use( plugins.controls );
 }

+export const jetpackBlocki18n = ( context, next ) => {


What do you think about calling this function here https://github.com/Automattic/wp-calypso/blob/master/client/gutenberg/editor/init.js#L25 before the extensions are loaded?

Well, I think I'll pass because:

We need access to the state, in order to fetch the user's current locale.

For the suggested approach, we need to pause the flow and continue after translations are loaded.

We could pass the state (or part of it) to initGutenberg from controller, but I agree that we shouldn't do it here because of your second point.

blowery · 2018-11-08T12:29:35Z

client/gutenberg/editor/controller.js

+	const languageFileUrl =
+		'https://widgets.wp.com/languages/jetpack-gutenberg-blocks/' + localeSlug + '.json';
+
+	request.get( languageFileUrl ).end( ( error, response ) => {


Minor terminology nit, this isn't a synchronous call (as laid out in the title and description of the PR), as it doesn't block the browser UI while it's inflight. That said, we should not block the browser UI by using a true synchronous request, so this is perfectly fine, unless you're expecting it to block the browser...

Yeah, good point, this is really sync-ish, because we don't prevent browser from loading anything, but in the same time we won't call next() until the translations request is finished.

tyxla · 2018-11-08T13:57:21Z

@tyxla just a note that I've recently refactored the way the editor is initialized in order to resolve some bugs, which might have some consequences for your approach here: #28302

Thanks @vindl, I've rebased the PR and it's still working as expected, so it's ready for another review 👀

ockham · 2018-11-22T18:07:55Z

Rebased.

ockham

Looks good, and tests well. While it'd be nice to have some more general-purpose solution in Gutenberg (see WordPress/gutenberg#12232), this is a reasonable solution for now. 🚢

As discussed on a video call with some Luna and Poseidon folks earlier today, we'll actually follow @sirreal's suggestion and use a similar pattern to fetch block visibility data (from @roccotripaldi's endpoint (D21225-code)). /cc @enejb

tyxla · 2018-11-23T14:32:26Z

I'd hold off merging for now - I prefer getting approval from @Automattic/i18n and @Automattic/lannister before going ahead with this one.

dmsnell · 2018-11-26T22:46:21Z

client/gutenberg/editor/controller.js

+	const languageFileUrl =
+		'https://widgets.wp.com/languages/jetpack-gutenberg-blocks/' + localeSlug + '.json';
+
+	request.get( languageFileUrl ).end( ( error, response ) => {


we shouldn't need to pull in superagent here and instead could consider using the data layer…

import { requestHttpData, waitForData } from 'state/data-layer/http-data'; import { rawHttp } from 'state/data-getters'; waitForData( { translations: requestHttpData( 'jetpack-gutenberg-translations', rawHttp( { method: 'GET', url: `https://widgets.wp.com/languages/jetpack-gutenberg-blocks/${ localeSlug }.json`, } ), { fromApi: () => ( { body } ) => [ [ 'jetpack-gutenberg-translations', body ] ] } ) } ).then( ( { translations: { state, data } ) => { if ( state === 'failure' ) { debug( `Encountered an error loading locale file for ${ localeSlug }. Falling back to English.` ); } // could still be expired data in the system if ( data ) { setLocaleData( data, 'jetpack' ); } next(); } );

granted, that's more code, but it's inspectable in our dev tools and comes with freshness and auto-retry policies and you can fake the data in testing easily enough. if you are interested I'd be happy to create a helper function in state/data-getters which cuts down on the overhead for doing a basic raw HTTP request

waitForData( { translations: getAtURL( `https://widgets.wp.com/…` ), } ).then( ( { translations: { state, data } } ) => { if ( 'failure' === state ) { debug( '…' ); } if ( data ) { setLocaleData( data.body, 'jetpack' ); } next(); } );

#28867

^^^ in case you are interested…

This is lovely, thanks @dmsnell ❤️

I've implemented your suggestion in this PR, and it's ready for another look.

See #28304 In this patch we're adding a small helper to be used to get raw information from a given URL though a `GET` request. The way we'd use this is inside of the `waitForData()` function. This is meant to obscure the global http-data ID so that the user doesn't need to worry about it. ```js waitForData( { planets: getAtURL( 'https://swapi.co/api/planets/' ), } ).then( ( { planets } ) => { console.log( planets.data ); } ); ```

tyxla · 2018-11-29T11:21:41Z

This is currently blocked by #28867.

See #28304 In this patch we're adding a small helper to be used to get raw information from a given URL though a `GET` request. The way we'd use this is inside of the `waitForData()` function. This is meant to obscure the global http-data ID so that the user doesn't need to worry about it. ```js waitForData( { planets: getAtURL( 'https://swapi.co/api/planets/' ), } ).then( ( { planets } ) => { console.log( planets.data ); } ); ```

See #28304 In this patch we're adding a small helper to be used to get raw information from a given URL though a `GET` request. The way we'd use this is inside of the `waitForData()` function. This is meant to obscure the global http-data ID so that the user doesn't need to worry about it. ```js waitForData( { planets: requestFromUrl( 'https://swapi.co/api/planets/' ), } ).then( ( { planets } ) => { console.log( planets.data ); } ); ```

ockham · 2018-12-03T10:39:29Z

Rebased

ockham · 2018-12-03T10:47:23Z

client/gutenberg/editor/controller.js

+	const localeSlug = getCurrentLocaleSlug( state );
+
+	// We don't need to localize English
+	if ( localeSlug === 'en' ) {


We might want to use config( 'i18n_default_locale_slug' ) here

ockham · 2018-12-03T11:14:28Z

Merging. Thanks Marin, Dennis, et al

dmsnell · 2018-12-04T00:51:42Z

Note that this is affected by the bug fixed in #29043

tyxla · 2018-12-05T18:25:04Z

Thanks for landing this one @ockham! 🙇

tyxla added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. i18n [Goal] Gutenberg Working towards full integration with Gutenberg labels Nov 6, 2018

tyxla self-assigned this Nov 6, 2018

tyxla requested review from a team November 6, 2018 12:09

vindl reviewed Nov 6, 2018

View reviewed changes

blowery reviewed Nov 8, 2018

View reviewed changes

tyxla force-pushed the try/gutenberg-calypso-jetpack-i18n-take-1 branch from 94fbc60 to 42bf6e5 Compare November 8, 2018 13:49

sirreal added the [Status] Needs Rebase label Nov 22, 2018

ockham mentioned this pull request Nov 22, 2018

FR: Async fetching prior to block registration WordPress/gutenberg#12232

Closed

ockham force-pushed the try/gutenberg-calypso-jetpack-i18n-take-1 branch from 42bf6e5 to 795e97e Compare November 22, 2018 18:07

ockham approved these changes Nov 22, 2018

View reviewed changes

gziolo mentioned this pull request Nov 23, 2018

Block and Component Lazy Loading WordPress/gutenberg#2768

Open

3 tasks

tyxla removed the [Status] Needs Rebase label Nov 23, 2018

dmsnell reviewed Nov 26, 2018

View reviewed changes

dmsnell mentioned this pull request Nov 26, 2018

Data Layer: Add helper to GET data from generic URL #28867

Merged

tyxla force-pushed the try/gutenberg-calypso-jetpack-i18n-take-1 branch from 2acd7cf to 373bd5b Compare November 28, 2018 09:32

tyxla added the [Status] Blocked / Hold label Nov 29, 2018

ockham added the [Pri] High label Nov 30, 2018

tyxla added 3 commits December 3, 2018 11:37

Gutenberg: Load Jetpack block translations in Calypso synchronously

f73ddb4

Do not localize English

b692e55

Replace superagent with waitForData and requestFromUrl

685a69b

ockham force-pushed the try/gutenberg-calypso-jetpack-i18n-take-1 branch from 373bd5b to 685a69b Compare December 3, 2018 10:38

ockham reviewed Dec 3, 2018

View reviewed changes

Use config( 'i18n_default_locale_slug' ) instead of 'en'

4f8f4cc

ockham merged commit 78430d0 into master Dec 3, 2018

ockham deleted the try/gutenberg-calypso-jetpack-i18n-take-1 branch December 3, 2018 11:14

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2018

mmtr mentioned this pull request Dec 3, 2018

Gutenlypso: Load translations #29026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gutenberg: Load Jetpack block translations in Calypso synchronously #28304

Gutenberg: Load Jetpack block translations in Calypso synchronously #28304

tyxla commented Nov 6, 2018 •

edited

Loading

matticbot commented Nov 6, 2018

vindl commented Nov 6, 2018

vindl Nov 6, 2018

tyxla Nov 8, 2018

vindl Nov 8, 2018

blowery Nov 8, 2018

tyxla Nov 8, 2018

tyxla commented Nov 8, 2018

ockham commented Nov 22, 2018

ockham left a comment

tyxla commented Nov 23, 2018

dmsnell Nov 26, 2018 •

edited

Loading

dmsnell Nov 26, 2018

tyxla Nov 27, 2018

tyxla commented Nov 29, 2018

ockham commented Dec 3, 2018

ockham Dec 3, 2018

ockham commented Dec 3, 2018

dmsnell commented Dec 4, 2018

tyxla commented Dec 5, 2018

Gutenberg: Load Jetpack block translations in Calypso synchronously #28304

Gutenberg: Load Jetpack block translations in Calypso synchronously #28304

Conversation

tyxla commented Nov 6, 2018 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Note

matticbot commented Nov 6, 2018

vindl commented Nov 6, 2018

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

tyxla commented Nov 8, 2018

ockham commented Nov 22, 2018

ockham left a comment

Choose a reason for hiding this comment

tyxla commented Nov 23, 2018

dmsnell Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Nov 29, 2018

ockham commented Dec 3, 2018

Choose a reason for hiding this comment

ockham commented Dec 3, 2018

dmsnell commented Dec 4, 2018

tyxla commented Dec 5, 2018

tyxla commented Nov 6, 2018 •

edited

Loading

dmsnell Nov 26, 2018 •

edited

Loading