Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 18 months ago

#55250 closed defect (bug) (fixed)

Avoid sending empty script translations

Reported by: jsnajdr's profile jsnajdr Owned by: ocean90's profile ocean90
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.0
Component: I18N Keywords: has-patch
Focuses: Cc:

Description

For every JS script enqueued, we generate an empty-ish translations script that doesn't add any translations:

<script id='react-js-translations'>
( function( domain, translations ) {
  var localeData = translations.locale_data[ domain ] || translations.locale_data.messages;
  localeData[""].domain = domain;
  wp.i18n.setLocaleData( localeData, domain );
}  )( "default", { "locale_data": { "messages": { "": {} } } } );
</script>

The only reason for that is to ensure that the wp.i18n library has a record for the domain in question (here "default"). But the wp.i18n library doesn't need that for __( 'foo', 'domain' ) to work, and it never needed.

There might have been some confusion about the Jed library, whose gettext methods really would throw an exception if the domain wasn't initialized, but 1) wp.i18n always caught the exception and returned the original string intact, and 2) it's been using tannin instead of Jed for close to 4 years.

This patch removes these redundant scripts. On a big page (like wp-admin/post.php for block editor on a site with many plugins), there would be around 90 of them.

Change History (10)

This ticket was mentioned in PR #2354 on WordPress/wordpress-develop by jsnajdr.


22 months ago
#1

Fixes https://core.trac.wordpress.org/ticket/55250

For every JS script enqueued, we generate an empty-ish translations script that doesn't add any translations:

<script id='react-js-translations'>
( function( domain, translations ) {
  var localeData = translations.locale_data[ domain ] || translations.locale_data.messages;
  localeData[""].domain = domain;
  wp.i18n.setLocaleData( localeData, domain );
}  )( "default", { "locale_data": { "messages": { "": {} } } } );
</script>

The only reason for that is to ensure that the wp.i18n library has a record for the domain in question (here "default"). But the wp.i18n library doesn't need that for __( 'foo', 'domain' ) to work, and it never needed.

There might have been some confusion about the Jed library, whose gettext methods really would throw an exception if the domain wasn't initialized, but 1) wp.i18n always caught the exception and returned the original string intact, and 2) it's been using tannin instead of Jed for close to 4 years.

This patch removes these redundant scripts. On a big page (like wp-admin/post.php for block editor on a site with many plugins), there would be 90 of them.

swissspidy commented on PR #2354:


22 months ago
#2

Note that the two unit tests Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency and Tests_Dependencies_Scripts::test_wp_set_script_translations_when_translation_file_does_not_exist need updating.

The change looks reasonable to me.

cc @ocean90 in case he has some reservations.

jsnajdr commented on PR #2354:


22 months ago
#3

The unit tests are now fixed 🎉 I also found the exact error that led to adding the empty translations in this comment: https://core.trac.wordpress.org/ticket/45103#comment:38

At that time (Nov 13 2018) we were still using Jed and when a domain was not registered, the i18n was logging an error to console (nothing else). What's a funny coincidence is that the `i18n` migration from Jed to Tannin was merged to Gutenberg on literally the next day 🙂

ocean90 commented on PR #2354:


21 months ago
#4

At that time (Nov 13 2018) we were still using Jed and when a domain was not registered, the i18n was logging an error to console (nothing else). What's a funny coincidence is that the `i18n` migration from Jed to Tannin was merged to Gutenberg on literally the next day 🙂

Thanks for digging into this! If there's no error anymore we should remove the unnecessary output.

#5 @ocean90
21 months ago

  • Milestone changed from Awaiting Review to 6.0

#6 @ocean90
21 months ago

  • Version changed from 5.9 to 5.0

#7 @ocean90
21 months ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 52937:

I18N, Script Loader: Don't register empty locale data objects.

For wp.i18n the library Jed was initially used which was throwing an error if a domain was not registered but used in a translate function. Later, the library was replaced by Tannin which no longer requires the domain to be registered and thus we can avoid printing an empty-ish translations script that doesn't add any translations.

Props jsnajdr.
Fixes #55250.

#8 @coffee2code
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change appears to have unintended side effects, breaking JS on a lot of w.org sites for any core-related JS functionality. Seems like the wp JS object doesn't get instantiated. See https://meta.trac.wordpress.org/ticket/6195.

#9 @ocean90
21 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Marking as fixed again. The research about the real cause is happing in #meta6195 and Gutenberg PR #39497.

Note: See TracTickets for help on using tickets.