i18n: `format-currency` - bring `formatCurrency` method to i18n-calypso #100097

chriskmnds · 2025-02-20T18:21:31Z

Part of addressing https://github.com/Automattic/i18n-issues/issues/939
Based off #100008

Proposed Changes

We bring the formatCurrency method to i18n-calypso to begin centralizing with the other number-formatters.

This does not bring the full format-currency package - there is still getCurrencyObject to migrate.
This does not deprecate formatCurrency function either. We will target one consumer scenario, the plan prices, to verify. We will do the rest in follow-ups.

This is a crucial refactor and creates the point from which we absolutely need to push through the remaining work (to bring currency formatting in full into i18n-calypso and deprecate format-currency). In this state, there is insane duplication and risk.

Why are these changes being made?

This is part of addressing https://github.com/Automattic/i18n-issues/issues/939, which aims at unifying number & currency-number formatting

TODO

Target pricing in the plans-grid as the first scenario.
Add also the tests from format-currency package.

Testing Instructions

unit tests
the plan billing descriptions on plans pages are now served through this new method:

go to /start/plans and /plans/:site
confirm the billing descriptions render correctly. Use different locale & currency combinations

For EL locale & USD:

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-02-20T18:21:44Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-134239

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-134239&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-134239&env=a8c-for-agencies

matticbot · 2025-02-20T18:25:57Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~2011 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-stepper              +5518 B  (+0.4%)    +1989 B  (+0.5%)
entry-main                 +5518 B  (+0.3%)    +1995 B  (+0.3%)
entry-subscriptions        +5487 B  (+0.3%)    +1973 B  (+0.4%)
entry-login                +5487 B  (+0.3%)    +1973 B  (+0.3%)
entry-domains-landing      +5487 B  (+0.8%)    +1973 B  (+1.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~17 bytes added 📈 [gzipped])

name                      parsed_size           gzip_size
update-design-flow              +43 B  (+0.0%)      +17 B  (+0.0%)
plugins                         +43 B  (+0.0%)      +17 B  (+0.0%)
plans                           +43 B  (+0.0%)      +17 B  (+0.0%)
jetpack-app                     +43 B  (+0.0%)      +17 B  (+0.0%)
async-step-unified-plans        +43 B  (+0.0%)      +17 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~17 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected        +43 B  (+0.0%)      +17 B  (+0.0%)
async-load-signup-steps-plans                          +43 B  (+0.0%)      +17 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot · 2025-02-20T18:28:26Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

blaze-dashboard
command-palette-wp-admin
happy-blocks
help-center
notifications
odyssey-stats
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/i18n-format-currency-add-to-i18n-calypso on your sandbox.

chriskmnds · 2025-02-20T18:38:52Z

packages/i18n-calypso/src/number-formatters/index.ts

@sirbrillig / @Automattic/i18n this will be the new (temporary) home for all number-formatters, form this point on including currency. The handlers themselves are raw, without state. The i18n-calypso instance/prototype simply lends its internal state for locale/geoLocation.

Once we migrate everything into this location, we can deprecate entirely format-currency and decide where to host number-formatters (it might move to a JP package or elsewhere). We can then introduce state handling to it if we want (or just keep using the handlers thorugh the i18n-calypso & JP number-format packages (which are basically portals/proxies).

sirbrillig

Looks pretty good! I noticed one small edge case that I should have brought up in #99981

packages/i18n-calypso/src/number-formatters/number-format-currency/index.ts

chriskmnds · 2025-02-24T13:26:35Z

packages/i18n-calypso/src/number-formatters/get-cached-formatter.ts

+		if ( retries ) {
+			return getCachedFormatter( {
+				locale: fallbackLocale,
+				options,
+				retries: retries - 1,
+			} );
+		}
+
+		throw error;


Added a limit on retries so it doesn't end up in an infinite loop - theoretically, as that's unlikely to happen with a valid fallback locale. Also failing loudly if for whatever reason it happens.

chriskmnds · 2025-02-24T13:29:31Z

packages/i18n-calypso/src/number-formatters/test/number-format-currency.ts

These tests are identical to format-currency package's, although they are done through the more primitive number-formatters interface (where we pass the locale and everything else as properties).

chriskmnds · 2025-02-24T13:31:29Z

packages/i18n-calypso/src/test/index.js

@@ -307,6 +307,73 @@ describe( 'I18n', function () {
 		} );
 	} );

+	describe( 'formatCurrency', () => {


The tests here combined with the primitive ones above should result in the same coverage as originally in format-currency package's. We basically test that the state handled properties (locale & geoLocation) propagate propagate properly into number-formatters.

chriskmnds · 2025-02-24T15:10:16Z

This is now ready for testing. The plan billing descriptions are set through this method now. See testing instructions.

Once we deploy here, the next couple of PRs will aim at bringing getCurrencyObject to i18n-calypso, migrate all exiting uses of formatCurrency and getCurrencyObject to use i18n-calypso, deprecate/remove format-currency from repo. From there, we will decide on the final location of number-formatters (whether JP or monorepo).

cc @sirbrillig / @Automattic/i18n

…llest-unit numbers

chriskmnds · 2025-02-24T15:15:21Z

ah sorry for the wide ping. rebased then changed parent here

sirbrillig

This works as described for me. The only thing I wasn't sure how to test is the logged-out plans page, and I'm not sure if that even uses these same code paths.

chriskmnds · 2025-02-25T05:57:59Z

This works as described for me. The only thing I wasn't sure how to test is the logged-out plans page, and I'm not sure if that even uses these same code paths.

Thanks @sirbrillig ! Did you mean the /pricing page? That's a whole different (sad) story. It's a different piece of code that's mostly rendered server-side.

p.s. since you mentioned I checked, and it looks like the currencies aren't localised properly or consistently there. I'll send these out in a P2 post so it gets looked at:

logged-out with EL (el. prefixed URL):

logged-in with EL locale - there is an attempt to localise the currencies (still not fine. missing the US):

and if logged-in and visit without the el. prefix, it looks a little weird (currencies localised, text isn't):

chriskmnds · 2025-02-25T10:23:37Z

This works as described for me. The only thing I wasn't sure how to test is the logged-out plans page, and I'm not sure if that even uses these same code paths.

Thanks @sirbrillig ! Did you mean the /pricing page? That's a whole different (sad) story. It's a different piece of code that's mostly rendered server-side.

p.s. since you mentioned I checked, and it looks like the currencies aren't localised properly or consistently there. I'll send these out in a P2 post so it gets looked at:

logged-out with EL (el. prefixed URL):

* logged-in with EL locale - there is an attempt to localise the currencies (still not fine. missing the `US`): * and if logged-in and visit without the `el.` prefix, it looks a little weird (currencies localised, text isn't):

Well actually, let me tag someone from @Automattic/martech-autobots. For context - in EL/DE, symbol follows the number e.g. 123 €.

chriskmnds self-assigned this Feb 20, 2025

chriskmnds added the i18n: number-format label Feb 20, 2025

chriskmnds requested a review from sirbrillig February 20, 2025 18:32

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 20, 2025

chriskmnds commented Feb 20, 2025

View reviewed changes

sirbrillig reviewed Feb 20, 2025

View reviewed changes

chriskmnds commented Feb 24, 2025

View reviewed changes

chriskmnds mentioned this pull request Feb 24, 2025

A4A: Format currency #100135

Open

8 tasks

chriskmnds marked this pull request as ready for review February 24, 2025 15:07

chriskmnds requested review from a team as code owners February 24, 2025 15:07

chriskmnds requested a review from jeyip February 24, 2025 15:07

chriskmnds added 6 commits February 24, 2025 17:13

i18n: format-currency - bring formatCurrency method to i18n-calypso

6684bf1

init with i18n.geolocatecurrencysymbol

104380d

cleanup

8c86d9a

fail early and ungracefully if precision cannot be determined for sma…

fb5278d

…llest-unit numbers

update plan billing descriptions to use new format-currency method

782e968

add tests. retries limit on get-cached-formatter

7eb8d35

chriskmnds force-pushed the update/i18n-format-currency-add-to-i18n-calypso branch from 33a6230 to 7eb8d35 Compare February 24, 2025 15:13

chriskmnds requested review from a team as code owners February 24, 2025 15:13

chriskmnds changed the base branch from update/i18n-format-currency-normalize-code to trunk February 24, 2025 15:14

sirbrillig approved these changes Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: `format-currency` - bring `formatCurrency` method to i18n-calypso #100097

i18n: `format-currency` - bring `formatCurrency` method to i18n-calypso #100097

chriskmnds commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited by matticbot

Loading

matticbot commented Feb 20, 2025 •

edited

Loading

matticbot commented Feb 20, 2025

chriskmnds Feb 20, 2025 •

edited

Loading

sirbrillig left a comment

chriskmnds Feb 24, 2025 •

edited

Loading

chriskmnds Feb 24, 2025

chriskmnds Feb 24, 2025 •

edited

Loading

chriskmnds commented Feb 24, 2025

chriskmnds commented Feb 24, 2025

sirbrillig left a comment

chriskmnds commented Feb 25, 2025 •

edited

Loading

chriskmnds commented Feb 25, 2025

i18n: format-currency - bring formatCurrency method to i18n-calypso #100097

Are you sure you want to change the base?

i18n: format-currency - bring formatCurrency method to i18n-calypso #100097

Conversation

chriskmnds commented Feb 20, 2025 • edited Loading

Proposed Changes

Why are these changes being made?

TODO

Testing Instructions

Pre-merge Checklist

github-actions bot commented Feb 20, 2025 • edited by matticbot Loading

matticbot commented Feb 20, 2025 • edited Loading

matticbot commented Feb 20, 2025

chriskmnds Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

sirbrillig left a comment

Choose a reason for hiding this comment

chriskmnds Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

chriskmnds Feb 24, 2025

Choose a reason for hiding this comment

chriskmnds Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

chriskmnds commented Feb 24, 2025

chriskmnds commented Feb 24, 2025

sirbrillig left a comment

Choose a reason for hiding this comment

chriskmnds commented Feb 25, 2025 • edited Loading

chriskmnds commented Feb 25, 2025

i18n: `format-currency` - bring `formatCurrency` method to i18n-calypso #100097

i18n: `format-currency` - bring `formatCurrency` method to i18n-calypso #100097

chriskmnds commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited by matticbot

Loading

matticbot commented Feb 20, 2025 •

edited

Loading

chriskmnds Feb 20, 2025 •

edited

Loading

chriskmnds Feb 24, 2025 •

edited

Loading

chriskmnds Feb 24, 2025 •

edited

Loading

chriskmnds commented Feb 25, 2025 •

edited

Loading