i18n: Implement number-formatters v1#42746
Conversation
|
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! |
Code Coverage Summary8 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
@Automattic/i18n I can't add/find you in Reviewers list. @dlind1 may be something you need to create/do on JP repo? Please also review the new package for any obvious code hit-misses. It's not linked anywhere, so nothing to test unless you really want to. @anomiex if you'd like to give this a review - is this the proper way of shipping a "alpha" release (or does it not matter and could just go for v1)? I'm branching off this PR to bring usage inside JP, so we'll be testing separately. |
|
@anomiex a NIT question/observation about setup: Could/should the |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the first version of the number-formatters package by porting functionality from i18n-calypso and introducing locale-aware number, compact number, and currency formatting methods. Key changes include the addition of TypeScript type definitions, a caching mechanism for Intl.NumberFormat instances, and comprehensive tests covering the new functionality.
Reviewed Changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/number-formatters/types/index.ts | Adds TypeScript interfaces and types for number formatting parameters and currency objects. |
| projects/js-packages/number-formatters/tests/ | Introduces new tests covering plain, compact, and currency formatting as well as caching behavior. |
| projects/js-packages/number-formatters/src/ | Implements number formatting methods, currency formatting with overrides, caching and a factory for creating formatters. |
| projects/js-packages/number-formatters/README.md | Updates documentation to reflect the new API and provides usage examples. |
| projects/js-packages/number-formatters/CHANGELOG.md | Documents the release version changes. |
Files not reviewed (2)
- pnpm-lock.yaml: Language not supported
- projects/js-packages/number-formatters/package.json: Language not supported
There was a problem hiding this comment.
if you'd like to give this a review - is this the proper way of shipping a "alpha" release (or does it not matter and could just go for v1)?
You're missing one thing that will stop it from actually releasing the alpha: when this is merged to trunk, the directory projects/js-packages/number-formatters/changelog/ needs to be empty of everything except the .gitkeep file. As it is now, that update-i18n-number-formatters-package file in there will prevent a release from being tagged.
The normal way to do it is to follow the instructions in PCYsg-Np7-p2 to make a PR that does only the cleaning out of the changelog/ dir and updating the CHANGELOG.md. As written it even uses a special branch ("prerelease") to avoid the possibility that someone might merge a conflicting PR right in the middle of your release.
Also of note:
- The autotagger won't tag versions that end in "-alpha" or "-a." (where
<num>is an even number), but "-alpha.1" should be ok. - The publish-to-NPM works on any version like "v*..", so as long as the tag gets tagged that should be good.
a NIT question/observation about setup: Could/should the
types/andtests/folders, and the mainindex.tsfile all migrate undersrc/folder? Just for my own knowledge, is there a convention/suggestion or is it pretty loose right now?
It's pretty loose. Our JS-related tooling is set up for several common conventions:
- Tests in files named
*.test.jsor*.spec.js(or jsx, ts, or tsx).- This can be alongside the sources, or in the
tests/directory.
- This can be alongside the sources, or in the
- Tests in directories like
**/__tests__/**/*.js - Tests in directories
**/test/*.js, usually alongside the sources.
Looks like in practice things are using either **/test/*.js style (and some are even **/test/*.test.js) or tests/**.test.js style. I see only three instances of *.test.js not under a test/ or tests/ directory, and no /__tests__/ or *.spec.js anywhere.
As far as types go, I don't think any of our tooling has any special configuration or requirements. Checking sources, the few types/ dirs I see are under src/ or an equivalent. I also see some .d.ts-named files not under a types/ dir. I don't know if there are also .ts files (no .d.) that happen to be types-only.
The main index.ts can go under src/ if you want it there. Again, I see both conventions in use.
|
Thanks @anomiex for the feedback. I certainly made some changes/ structural changes based on what you mentioned.
So I removed the entry/version from the changelog. From what I understand, I should follow up with a PR now doing the changelog cleanup and adding the new version, per PCYsg-Np7-p2. 🤔
Got it, thanks for the additional notes. The reason I'm going with |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the v1 number-formatters package for formatting regular numbers, compact numbers, and currencies with locale-aware customizations. Key changes include:
- Removing legacy tests from projects/js-packages/number-formatters/tests/format-number.test.ts.
- Introducing new files and tests in the src/ folder to support refined formatting logic for numbers and currencies.
- Structural updates to improve caching, locale handling, and error fallback behavior.
Reviewed Changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/format-number.test.ts | Removed legacy tests, presumably replaced by tests in the src/test folder. |
| src/types.ts | Added detailed type definitions for formatter parameters and currency objects. |
| src/test/* | New test files covering various formatting functions. |
| src/number-format.ts | Implements basic number formatting with locale and option integration. |
| src/number-format-currency/index.ts | Provides extensive currency formatting logic with overrides and fallback handling. |
| src/create-number-formatters.ts | Creates a closure-based factory managing locale state and exposing formatter methods. |
| README.md & CHANGELOG.md | Updated documentation and changelog to reflect the new v1 functionality. |
Files not reviewed (4)
- pnpm-lock.yaml: Language not supported
- projects/js-packages/number-formatters/.gitattributes: Language not supported
- projects/js-packages/number-formatters/package.json: Language not supported
- projects/js-packages/number-formatters/tsconfig.json: Language not supported
Comments suppressed due to low confidence (1)
projects/js-packages/number-formatters/tests/format-number.test.ts:1
- The removal of this test file means that the functionality of formatNumber should be verified elsewhere. Please ensure that equivalent tests are present in the new test suite, so that no regressions occur.
import { describe, expect, test } from '@jest/globals';
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
c5c11ce to
fcee859
Compare
|
Looks reasonable to me from a structural perspective. I didn't review the code at all though.
Yes. 🙂 |
|
The test coverage seems to only be lacking in the main index file, where the exports happen. I can add a test later to confirm that what's expected is returned. |
See PT: pxLjZ-9ME-p2
Part of addressing https://github.com/Automattic/i18n-issues/issues/942
Proposed changes:
number-formatterspackage #42639 and brings the implementation ofnumber-formattersfrom i18n-calypsoIntl.NumberFormatSee the package's README for details on the API, usages, and some dev/workflow notes.
Setup
We try to keep a similar functional separation with
Intl.NumberFormat( ... ).format( ... ):Intl.NumberFormat()localeto be passed as a parameter (we don't currently export these, but we can down the line)number-format-currency. It returns the formatted number, so it's essentially a fullIntl.NumberFormat( ... ).format( ... )call. It may receive a refactor to sync down the line, which will idealy improve some of the code sharinglocalestate, returning abstractions over the number formatters from above, calledformaNumber,formatNumberCompact, etc.i18n-calypso, which will make migration in wp-calypso easy(er)decimalsprop)Adding a new formatter to the mix, like for "percentages", would basically just repeat what we've done for "compact" numbers: a method returned from
create-number-formatters.tsand a function innumber-format.ts(or separate file if anything deeper than a few simple defaults).TODO
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: