i18n: Provision new number-formatters package#42639
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! |
94ad3d2 to
e5ec00c
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage Summary2 files are newly checked for coverage.
|
anomiex
left a comment
There was a problem hiding this comment.
In order for the monorepo tooling to work, you'll need to add a few additional files.
-
A
composer.jsonwith some configuration settings.Details
This should do it:
{ "name": "automattic/js-number-formatter", "description": "Number formatting utilities.", "license": "GPL-2.0-or-later", "require": {}, "require-dev": { "automattic/jetpack-changelogger": "@dev" }, "scripts": { "build-production": [ "pnpm run build" ], "build-development": [ "pnpm run build" ], "test-js": [ "pnpm run test" ], "test-coverage": [ "pnpm run test-coverage" ] }, "repositories": [ { "type": "path", "url": "../../packages/*", "options": { "monorepo": true } } ], "extra": { "autotagger": true, "npmjs-autopublish": true, "mirror-repo": "Automattic/number-formatter", "textdomain": "number-formatter", "changelogger": { "link-template": "https://github.com/Automattic/number-formatter/compare/${old}...${new}" } } }The
namefield here doesn't matter. The really important stuff is inscriptsandextra.Of particular note in there is the reference to a mirror repo, which as specified would be https://github.com/Automattic/number-formatter. If you'd rather have a different mirror repo name (under the Automattic organization), that's fine. Once the name is settled, I can create it for you or you can try following the instructions in https://github.com/Automattic/jetpack/blob/trunk/docs/monorepo.md#mirror-repositories. 🙂
-
An empty file at
changelog/.gitkeep. -
The
.gitignorefile mentioned in one of the inline comments.
There was a problem hiding this comment.
And finally for the setting things up correctly, you'll need to have this look something like
| { | |
| "extends": "jetpack-js-tools/tsconfig.base.json", | |
| "include": [ "./index.ts", "./src" ] | |
| } | |
| { | |
| "extends": "jetpack-js-tools/tsconfig.tsc.json", | |
| "include": [ "./index.ts" ], | |
| "compilerOptions": { | |
| "typeRoots": [ "./node_modules/@types/" ], | |
| "sourceMap": false, | |
| "outDir": "./build/", | |
| "target": "es2022", | |
| } | |
| } |
You're welcome to use whichever target is appropriate, rename "build" to "dist" if you prefer that, etc.
There was a problem hiding this comment.
Ok. Added. Indeed I changed "build" to "dist".
The only problem is changing "extends": "jetpack-js-tools/tsconfig.base.json" to "extends": "jetpack-js-tools/tsconfig.tsc.json". It gives the error I mentioned above
There was a problem hiding this comment.
Ok - my replies from the editor may not be coming through. I said earlier:
with the suggested change in "extends", I get the following error in index.ts:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './src/format-number.js'?
There was a problem hiding this comment.
That's expected, and you should make the change it calls for.
What's going on here is that Node requires the extension for esm imports, while bundlers like webpack typically don't.
tsc doesn't try to rewrite the imports at all, it just outputs what its given. If the tsconfig has moduleResolution set to "nodenext" rather than "bundler", it issues that error to flag that the import won't work if the output is going to be run by Node. When you're writing a library like this one, you don't know whether the project using your library is going to be using "nodenext" or "bundler", so TypeScript recommends using the stricter setting for best compatibility.
There was a problem hiding this comment.
That's interesting. Thanks for explaining. I did come across this in the long distant past. I need to get my head around it.
I don't exactly understand this part:
When you're writing a library like this one, you don't know whether the project using your library is going to be using "nodenext" or "bundler"
How do I not know that? Does that make every package we export from wp-calypso faulty in that sense (cause this is the first time I come across this)?
Sorry for my ignorance. I'll try and sum it up in my head
There was a problem hiding this comment.
Does that make every package we export from wp-calypso faulty in that sense (cause this is the first time I come across this)?
Maybe? Or maybe I've misunderstood something at some point. But most likely it just hasn't come up because either all reusers are processing the packages via a bundler like webpack or they're using tsc with moduleResolution set to "bundler" (and aren't running the package via node directly either).
There was a problem hiding this comment.
Interesting. That may well be the case. Thanks for clarifying and sharing the links above.
e5ec00c to
563eeb3
Compare
e95bec2 to
668eceb
Compare
|
@anomiex Thanks for the help/feedback! :-) I've addressed most comments and left a couple of questions above. I'd be happy to get a bare-bones package shipped (with no more functionality), just to have a base-line setup to refer to. I'd like that if acceptable. Bringing more code into this PR may require additional dependencies, etc. |
pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
Something seems to be going wrong with this file, it's getting massively reformatted.
Your best bet may be to git checkout origin/trunk pnpm-lock.yaml and then pnpm dedupe to regenerate it.
There was a problem hiding this comment.
Since there's no link for the 0.1.0 version in the file, we need to delink it here.
| ## [0.1.0] - 2024-03-18 | |
| ## 0.1.0 - 2024-03-18 |
|
@anomiex you are trully awesome. Thanks for the continued help and feedback :-) |
|
I think this looks good to go now, thanks to @anomiex. I created the mirror repo here, although I couldn't figure out steps 4 and 6 from the instruactions:
I created the ruleset, but that's as far as I got. I couldn't find the Automattic Bot app to add to the bypass list (if that's what I'm supposed to do)
|
|
The step about matticbot usually doesn't actually need you to do anything. I'll go ahead and add the secrets. |
anomiex
left a comment
There was a problem hiding this comment.
You still need to add a .gitignore file with contents like
/dist/
83ef570 to
4de61b4
Compare
|
Thanks @anomiex. Up to you how best to proceed from here. We can flesh this out into the full package or ship the skeleton and add the logic next. Come to think of it, it doesn't make a lot of difference on my end :-)
👍 |
anomiex
left a comment
There was a problem hiding this comment.
Looks good to me to merge and iterate.
If you want a version actually published to npm after merging this, you'll want to follow the instructions in PCYsg-Np7-p2 to have a release tagged. Feel free to ask in Slack (#jetpack-releases) if you want help with that.
|
@anomiex Thanks again! I'll follow up on the release from here |
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
4de61b4 to
2181f78
Compare
|
I get these E2E tests consistently failing, although they don't look related. Do I proceed with merging? I'm assuming a similar wp-calypso process - with "required" being the "go" flag. |
|
Looks like the "Push to mirror repos" job failing https://github.com/Automattic/jetpack/actions/runs/14065109246/job/39386553959 |
|
Yeah, "required" is the real blocker. Unfortunately some of these E2Es are a bit flaky and no one has stepped up to fix them.
Whatever you did for the "Make sure that matticbot can push to the repo" step somehow prevented it. 😅 Really, for repos under the Automattic organization, it just works by default anyway. I made #42702 to update that doc, and fixed the mirror repo settings so the re-run at https://github.com/Automattic/jetpack/actions/runs/14065109246/job/39388963330 worked. 👍 |
| ## Usage | ||
|
|
||
| ```typescript | ||
| import { formatNumber } from '@automattic/jetpack-number-formatters'; |
There was a problem hiding this comment.
Oops, we missed this earlier. 😅 Something for a followup.
| import { formatNumber } from '@automattic/jetpack-number-formatters'; | |
| import { formatNumber } from '@automattic/number-formatters'; |
There was a problem hiding this comment.
Gotcha. We'll have a PR soon with lots of changes here :-)
| "jetpack:src": "./index.ts", | ||
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| } | ||
| }, | ||
| "scripts": { | ||
| "build": "pnpm run clean && pnpm run compile-ts", | ||
| "clean": "rm -rf dist/", | ||
| "compile-ts": "tsc --pretty", |
There was a problem hiding this comment.
Do we really need a build step here?
Is the package used outside Jetpack? If not, then may be we can only have "jetpack:src" and remove everything else?
There was a problem hiding this comment.
Hi!
Is the package used outside Jetpack?
Yes, intended for use outside.
There was a problem hiding this comment.
Personally, I'd say if it has TS and we're publishing it to npm then it probably should have a build script set up. If we really don't want it to be possibly used externally, we probably shouldn't be publishing it in the first place.
See PT: pxLjZ-9ME-p2
Part of addressing https://github.com/Automattic/i18n-issues/issues/942
Proposed changes:
Introduces a new js-package
number-formattersfor formatting numbers/currencies/percentages/etc. in a unified way across wpcom frontends.This is a skeleton PR with base configs to provision the new package.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: