Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use the new i18n-loader-webpack-plugin #21936

Merged
merged 12 commits into from
Dec 15, 2021
Merged

Use the new i18n-loader-webpack-plugin #21936

merged 12 commits into from
Dec 15, 2021

Conversation

anomiex
Copy link
Contributor

@anomiex anomiex commented Dec 2, 2021 •
edited
Loading

Changes proposed in this Pull Request:

  • webpack-config: Use @automattic/i18n-loader-webpack-plugin

    Provide the new plugin as part of the standard set of plugins.

  • jetpack: Use @automattic/i18n-loader-webpack-plugin

    Configure the new plugin and remove the hack Instant Search was using to
    work around the problem the plugin was created to fix.

Jetpack product discussion

PT: p9dueE-3MG-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Is CI happy?
  • Configure your site for a non-English locale, activate Instant Search, then search. Is the text in Instant Search's popup translated?
    • If you're using the Jetpack Docker env, you might do jetpack docker exec /var/scripts/fakepot.sh /var/www/html/wp-content/plugins/jetpack to generate en_piglatin and en_rtl locales.
      ss
  • Do the above on wpcom too.
    • Currently to test it on a wpcom sandbox, you'll also have to go into wp-content/mu-plugins/jetpack-packages/production/ and composer update automattic/jetpack-assets=1.14.x-dev to get the new version of the assets package.
    • Also add this filter hook somewhere: 
      function wpcom_jetpack_i18n_state( $data ) {
    $data['baseUrl'] = 'https://widgets.wp.com/languages/';
    return $data;
}
add_filter( 'jetpack_i18n_state', 'wpcom_jetpack_i18n_state' );
      (that'll have to be added to D70962-code before we merge it)

@anomiex anomiex requested a review from a team as a code owner December 2, 2021 19:21
@anomiex anomiex self-assigned this Dec 2, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello anomiex! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D70962-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@anomiex anomiex added [Pri] Normal [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed Touches WP.com Files labels Dec 2, 2021
@github-actions github-actions bot added [JS Package] Webpack Config [Package] Assets [Package] Connection UI This package no longer exists in the monorepo. [Package] Identity Crisis This package no longer exists in the monorepo. It was merged into [Package] Connection. [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Package] My Jetpack [Package] Post List [Package] Search Contains core Search functionality for Jetpack and Search plugins [Package] Tracking This package no longer exists in the monorepo. It was merged into [Package] Connection. [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ RNA [Feature] Search For all things related to Search labels Dec 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2021 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: January 11, 2022.
  • Scheduled code freeze: January 3, 2022.

Backup plugin:

  • Next scheduled release: January 4, 2022.
  • Scheduled code freeze: December 27, 2021.

kangzj
kangzj previously approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@kangzj kangzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anomiex wonderful job! So excited we could get this resolved so soon.
This works for me and looks good. We might want to test WPCOM before merging, left an inline comment.

projects/plugins/jetpack/modules/search/class-jetpack-instant-search.php
wp_register_script( $handle, $payload_url, array(), false, false );
// Set translation domain to `jetpack`, and we need to explicitly set the `path` to load translations files for WPCOM.
// Otherwise WPCOM would try to load from `WP_LANG_DIR . '/mu-plugins'` and fails.
wp_set_script_translations( $handle, 'jetpack', WP_LANG_DIR . '/plugins' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in WPCOM, the translation path was resolved to mu-plugins if /plugins not specified here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ok since the path now isn't being determined by wpcom PHP code, but testing is a good idea.

Looks like we may have to wait until #21932 is merged first so the TC build on D70962-code will succeed. Or merge this into #21932 then test it with a D70957-code that contains the changes from here too, if reviewers would rather.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out testing this is kind of a pain, because the Fusion patch doesn't include the changes to the Assets package but those are required for it to work.

Once I manually apply those in my sandbox, it almost works. It tries to fetch the right path, but apparently wpcom blocks access to .json files inside wp-content/languages/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out, and updated the testing instructions so it should be easy for someone else to test it too.

Base automatically changed from add/i18n-loader-webpack-plugin to master December 9, 2021 14:44
@anomiex anomiex removed [Package] Jitm [Package] Identity Crisis This package no longer exists in the monorepo. It was merged into [Package] Connection. [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Package] Post List [Package] Search Contains core Search functionality for Jetpack and Search plugins [Package] My Jetpack labels Dec 14, 2021
@anomiex
Copy link
Contributor Author

anomiex commented Dec 14, 2021

Sigh. So the wpcom stuff isn't happy because D71392-code isn't merged yet, but it can't be due to p3topS-X5-p2

@anomiex
Copy link
Contributor Author

anomiex commented Dec 15, 2021

Hacked around the wpcom problem, this should be ready for review again.

sdixon194
sdixon194 previously approved these changes Dec 15, 2021
View reviewed changes
@sdixon194 sdixon194 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 15, 2021
@github-actions github-actions bot added [Package] Connection UI This package no longer exists in the monorepo. [Package] Identity Crisis This package no longer exists in the monorepo. It was merged into [Package] Connection. [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Package] My Jetpack [Package] Search Contains core Search functionality for Jetpack and Search plugins [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. labels Dec 15, 2021
@anomiex anomiex merged commit 30d163d into master Dec 15, 2021
@anomiex anomiex deleted the add/use-i18n-loader-webpack-plugin branch December 15, 2021 19:00
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D70962-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 15, 2021
@anomiex
Copy link
Contributor Author

anomiex commented Dec 15, 2021

r236678-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

@sdixon194 sdixon194 sdixon194 left review comments

@kangzj kangzj kangzj left review comments

Assignees

@anomiex anomiex

Labels
[Feature] Search For all things related to Search [JS Package] I18n Loader Webpack Plugin [JS Package] Webpack Config [Package] Connection UI This package no longer exists in the monorepo. [Package] Identity Crisis This package no longer exists in the monorepo. It was merged into [Package] Connection. [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Package] My Jetpack [Package] Search Contains core Search functionality for Jetpack and Search plugins [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal RNA Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anomiex @matticbot @kraftbj @kangzj @sdixon194 @bluefuton