i18n: Update formatCurrency / getCurrencyObject imports to be from @automattic/number-formatters#42796
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the currency formatting imports across the codebase to use the new package (@automattic/number-formatters) instead of the legacy @automattic/format-currency. These changes ensure that both formatCurrency and getCurrencyObject are consistently imported from the new package, aligning with the updated initialization requirements.
Reviewed Changes
Copilot reviewed 19 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/extensions/blocks/donations/deprecated/v1/index.js | Replaced default and named imports from the old package with separate imports for CURRENCIES and formatCurrency. |
| projects/plugins/jetpack/extensions/blocks/donations/amount.js | Updated the imports for formatCurrency and CURRENCIES to their new sources. |
| projects/plugins/jetpack/_inc/client/recommendations/product-card-upsell/index.jsx | Swapped getCurrencyObject import to use the new package. |
| projects/plugins/jetpack/_inc/client/recommendations/feature-utils.js | Removed old formatCurrency import and added the new import from @automattic/number-formatters. |
| projects/plugins/jetpack/_inc/client/components/jetpack-product-card/test/component.js | Updated getCurrencyObject import for consistency in test files. |
| projects/plugins/boost/app/assets/src/js/features/upgrade-cta/upgrade-cta.tsx | Replaced getCurrencyObject import with the new package import. |
| projects/packages/search/src/dashboard/components/price/index.jsx | Updated getCurrencyObject import accordingly. |
| projects/packages/my-jetpack/_inc/components/product-detail-card/index.jsx | Adjusted getCurrencyObject import to align with new formatting conventions. |
| projects/packages/my-jetpack/_inc/components/product-card/pricing-component.tsx | Replaced formatCurrency import with the new package version. |
| projects/packages/backup/src/js/components/backup-storage-space/storage-addon-upsell-prompt/price.js | Updated the getCurrencyObject import to use the new package. |
| projects/js-packages/components/components/product-price/price.tsx | Changed getCurrencyObject import to the new package import. |
| projects/js-packages/components/components/pricing-card/index.tsx | Updated getCurrencyObject import to maintain consistency across components. |
Files not reviewed (8)
- projects/js-packages/components/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/js-packages/number-formatters/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/packages/backup/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/packages/my-jetpack/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/packages/my-jetpack/package.json: Language not supported
- projects/packages/search/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/plugins/boost/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/plugins/jetpack/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
projects/js-packages/number-formatters/changelog/update-i18n-number-formatters-formatCurrency
Outdated
Show resolved
Hide resolved
c5c11ce to
fcee859
Compare
8a118ce to
68b9fe3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the currency formatting imports to use the new @automattic/number-formatters package and adjusts the underlying initialization and fallback logic for locale settings.
- Updated import statements for formatCurrency and getCurrencyObject across several files.
- Modified create-number-formatters.ts to use state variables (localeState and geoLocationState) and a fallback chain via getBrowserSafeLocale.
- Ensured consistency in how locale information is derived using the new package's API.
Reviewed Changes
Copilot reviewed 18 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/_inc/client/recommendations/product-card-upsell/index.jsx | Updated getCurrencyObject import |
| projects/plugins/jetpack/_inc/client/recommendations/feature-utils.js | Updated formatCurrency import |
| projects/plugins/jetpack/_inc/client/components/jetpack-product-card/test/component.js | Updated getCurrencyObject import in tests |
| projects/plugins/boost/app/assets/src/js/features/upgrade-cta/upgrade-cta.tsx | Updated getCurrencyObject import |
| projects/packages/search/src/dashboard/components/price/index.jsx | Updated getCurrencyObject import |
| projects/packages/my-jetpack/_inc/components/product-detail-card/index.jsx | Updated getCurrencyObject import |
| projects/packages/my-jetpack/_inc/components/product-card/pricing-component.tsx | Updated formatCurrency import |
| projects/packages/backup/src/js/components/backup-storage-space/storage-addon-upsell-prompt/price.js | Updated getCurrencyObject import |
| projects/js-packages/number-formatters/src/create-number-formatters.ts | Updated locale and geoLocation logic |
| projects/js-packages/components/components/product-price/price.tsx | Updated getCurrencyObject import |
| projects/js-packages/components/components/pricing-card/index.tsx | Updated getCurrencyObject import |
Files not reviewed (9)
- pnpm-lock.yaml: Language not supported
- projects/js-packages/components/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/js-packages/number-formatters/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/js-packages/number-formatters/package.json: Language not supported
- projects/packages/backup/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/packages/my-jetpack/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/packages/my-jetpack/package.json: Language not supported
- projects/packages/search/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
- projects/plugins/boost/changelog/update-i18n-number-formatters-formatCurrency: Language not supported
Comments suppressed due to low confidence (1)
projects/js-packages/number-formatters/src/create-number-formatters.ts:150
- [nitpick] Consider renaming 'localeState' to 'browserSafeLocale' (or a similar name) to more clearly reflect its purpose in normalizing and providing the locale for formatting.
localeState = locale;
6e41270 to
4b979f7
Compare
|
All the checks pass, and I tested a couple of cases (in description). There are obviously more usages, but I don't have a fully activated local environment yet e.g., an upsell with price rendering in the Backup plugin I can't get to yet. I'll be doing more testing for the other PRs too. Ok to go with this? @anomiex / @Automattic/martech |
anomiex
left a comment
There was a problem hiding this comment.
Looks reasonable to me (flagged one nitpick though). But I didn't try to test any of the functionality.
| "webpack-cli": "6.0.1" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Why remove the newline from the end of the file?
Same in other files below.
There was a problem hiding this comment.
I am not sure. It's not flagged as a violation for package.json files, apparently. I have my editor set up to apply linting when saving, so these get removed (I think the root issue is elsewhere though). Something to do with the lining configuration perhaps, cause it's not the case in Calypso (for example - .json files are parsed with eol-last like regular JS).
There was a problem hiding this comment.
Updated. But curious why this isn't part of linting. It should have been flagged
There was a problem hiding this comment.
We don't have any eslint set up for package.json files. Yet, anyway.
Probably your editor or something saves them without the trailing newline, then in Calypso linting fixes it.
See PT: pxLjZ-9ME-p2
Part of addressing https://github.com/Automattic/i18n-issues/issues/942
Proposed changes:
This PR follows from the work in #42746 to implement the new
@automattic/number-formatterspackage.formatCurrnecy&getCurrencyObjectimports from@automattic/format-currencyto go through the new@automattic/number-formatterspackageThis does not deprecate
@automattic/format-currencyyet. There are remaining usages with obsolete methods, something that will be handled elsewhere e.g. in #42456 and #42457Media
Pricing cards e.g. at
/wp-admin/admin.php?page=my-jetpack#/add-backup.decimal separator),decimal separator)Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Follow the instructions to run JP in Docker and build the Jetpack plugin. Once the env is ready, the rest boils down to:
cp tools/docker/default.env tools/docker/.envjp docker up -dcp tools/docker/default.env tools/docker/.envjp docker install- may need to wait a bit firstjp build plugins/jetpack --deps- this will take a whilejp watch- obviously for testing/debugging thingsExample – The pricing/plan cards for getting a plan for certain plugins under my-jetpack:
/wp-admin/admin.php?page=my-jetpack, then click on "Get Plan" under "VaultPress Backup"/wp-admin/admin.php?page=my-jetpack#/add-backup/wp-admin/profile.phpand switch locale/language between Greek/Deutsch, and English.