number-formatters: build dual CJS and ESM versions

[jsnajdr]

This PR adds a Rollup build to the number-formatters package and makes it produce both CJS and ESM versions of the module. Also adds the "legacy" main, module and types fields to package.json in addition to the modern exports field.

This makes the package consumable both by modern and legacy tooling setups.

The Rollup workflow is modelled after what we already have in the @automattic/charts package. I basically copied the config and then simplified it a lot. We don't need any CSS support in number-formatters for example. And also I think that Terser minification doesn't need to be done. It's up to the final consumer to do all the minification at once.

When I use the resulting package in Automattic/wp-calypso#102475, all the errors reported there disappear: Jest is able to require the CJS version, and also TypeScript is able to consume the package without problems.

Testing instructions:

1. Verify that the output of pnpm run build in the dist directory is valid and makes sense.
2. Try to use this package in i18n: Bring @automattic/number-formatters – use in section stats > insights wp-calypso#102475 (I just copied over package.json and dist) and verify that it fixes the yarn test-client and yarn typecheck errors.

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

No.

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the update/number-formatters-rollup-build branch.
• To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/number-formatters-rollup-build

bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/number-formatters-rollup-build

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions]

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.
  
• ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  
• ✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  
• ✅ 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 🤖

---

Follow this PR Review Process:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
3. You can use GitHub's Reviewers functionality to request a review.
4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

[jp-launch-control]

Code Coverage Summary

This 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. 🤷

Full summary · PHP report · JS report

[anomiex]

I'd really rather we don't add more use of rollup. The configs we have tend to be unmaintained, and to depend on unmaintained plugins, and throw warnings which are ignored. This one, for example, issues this warning:

(!) Plugin typescript: @rollup/plugin-typescript TS5110: Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.

Is anyone committing to maintaining the rollup configuration here, including replacing outdated plugins and such?

For a library we probably shouldn't need a bundler at all, just tsc to convert the TypeScript to JavaScript.

I also find that the resulting files in dist/cjs/ are not correct. The package.json has "type": "module", indicating that files such as dist/cjs/index.js are supposed to be in ESM rather than CommonJS format. While some tooling may ignore this mismatch, other tooling will fail. To work correctly, these files need to be named like dist/cjs/index.cjs instead.

[anomiex]

I find that blanket-listing directories (rather than listing the specific entry point files such as src/index.ts) in a tsconfig that's used for compilation tends to give odd results.

In this case we seem to be ok because everything is reachable from the entry point, but in other packages I've seen this pointlessly compile .test.js and .stories.js files.

[anomiex]

Typescript calls for "types" to come before "import" and "require".

Suggested change

	"import": "./dist/mjs/index.js",
	"require": "./dist/cjs/index.js",
	"types": "./dist/index.d.ts"
	"types": "./dist/index.d.ts",
	"import": "./dist/mjs/index.js",
	"require": "./dist/cjs/index.js"

[jsnajdr]

Thanks for your feedback @anomiex 👍 I'm getting rid of Rollup and building the dual package with TypeScript instead. There is one tsconfig.json for ESM and I'm making all the necessary config changes with CLI arguments when building the CJS version.

The only thing that doesn't work yet are .cjs extensions for the CJS build so that Node doesn't treat them as ES modules. TypeScript cannot do it natively (see microsoft/TypeScript#49462) and I'll need to figure out some other way to rename .js to .cjs.

The little issues reported (order of the types condition, no blanket include of entire src dir) are fixed.

[anomiex]

I'm wary of this. the customConditions are what makes

jetpack/projects/js-packages/number-formatters/package.json

Line 36 in 978c9d4

"jetpack:src": "./src/index.ts",

work.

At the moment, disabling it here is probably ok, since this package doesn't depend on anything else within the monorepo. And maybe this package never will. OTOH, it may be a bad pattern since this may get copied into other packages and break things there.

[anomiex]

I'm pretty wary of this too. https://www.typescriptlang.org/docs/handbook/modules/reference.html#commonjs, for example, says "You probably shouldn’t use this."

It seems the more modern way to do something like this, in the absence of offical support in tsc, is to somehow munge the "type" in package.json just before the build to have tsc with "nodenext" build the cjs version, then restore it after. For example, it looks like @knighted/duel does that directly, while tshy assumes sources are in src/ and creating a temporary stub src/package.json will manage to DTRT. (of those two, BTW, duel seems more of the "do one specific thing" philosophy while tshy seems more "try to do all the things".)

[jsnajdr]

I think I addressed both concerns by creating a special src/package.json during the CommonJS build, with type: 'commonjs'. Just like tshy does. It's easy to do, just a cp and rm command.

We don't need to remove customConditions any more, because module and moduleResolution are always nodenext. It's the type field that triggers the CommonJS build.

[jsnajdr]

Ensuring that files inside dist/cjs have .cjs extensions is hard. The easy part is to rename the files, but the hard part is updating all the require( './something.js' ) statements in the built files to require( './something.cjs' ).

@anomiex What do you think about using @knighted/duel for the build? They can parse all the files and update the require syntax, they have a separate @knighted/specifier package for that. The tool does complex things, it's hard to replicate it with a simple package.json script 🙁

And the .cjs extensions are really important to have, without them Node.js fails to require the package:

Error [ERR_REQUIRE_ESM]: require() of ES Module dist/cjs/index.js not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead either rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

[anomiex]

I think, if we really do want a dual package, that using @knighted/duel to build it seems reasonable. I'd recommend the --dirs option, it seems cleaner that way.

[jsnajdr]

@anomiex I migrated the build to @knighted/duel and it looks like a success 🙂 The CI checks are green and I believe we are ready to approve and merge.

[anomiex]

Looking good. Left a few suggestions for cleanup inline.

[anomiex]

What's this for?

... Ah, missing upstream peer dependency. Sigh. Thanks for filing https://href.li/?https://github.com/knightedcodemonkey/duel/issues/63 about it.

Personally I might be inclined to work around this by adding a hack to .pnpmfile.cjs (in the monorepo root).

// Package is missing a dependency for one of its subpackages that has this as a peer dep.
// https://github.com/knightedcodemonkey/duel/issues/63
if ( pkg.name === '@knighted/duel' && ! pkg.dependencies?.[ 'oxc-parser' ] ) {
    pkg.dependencies[ 'oxc-parser' ] = '*';
}

That lets us better document that the dep is only there because of an upstream bug.

(then remove it from package.json here)

[anomiex]

Looks like upstream has released a version with a fix already 🎉 So we can just remove this dep now. 🙂

[anomiex]

I'd probably do like this

Suggested change

	"build": "pnpm run clean && duel --dirs",
	"clean": "rm -rf dist",
	"compile-ts": "tsc --pretty",
	"build": "pnpm run clean && pnpm run compile-ts",
	"clean": "rm -rf dist",
	"compile-ts": "duel --dirs",

But this would also be fine.

Suggested change

	"build": "pnpm run clean && duel --dirs",
	"clean": "rm -rf dist",
	"compile-ts": "tsc --pretty",
	"build": "pnpm run clean && duel --dirs",
	"clean": "rm -rf dist",

[anomiex]

While we're here, it might be worth making these deps be like

		"@wordpress/date": "^5.19.0",
		"debug": "^4.4.0",
		"tslib": "^2.5.0"

That way reusers don't have to have these exact versions of the dependencies, if they have e.g. tslib 2.8.1 elsewhere in their project then they won't need this older version too just for this package.

outdated, remaining requests aren't really blockers

[chriskmnds]

Thank you both for working on this! 🙇

I think once (or while) we ship this, we can also change version to 1.0.0 and do a release on NPM. Then I can update the wp-calypso PRs to use 1.0.0. Or alternatively, we can do 1.0.0-alpha.3 until I confirm things working in JP, then do 1.0.0 release (and call it a day then)

These PRs are waiting for this bit to complete, which should complete migration in wp-calypso (then back to JP):

1. i18n: Bring @automattic/number-formatters – use in section stats > insights wp-calypso#102475
2. i18n: number-formatters – handle callback in geolocateCurrencySymbol wp-calypso#102538
3. i18n: number-formatters – transform client components/files to use @automattic/number-formatters wp-calypso#102510
4. i18n: number-formatters – transform packages & apps to use @automattic/number-formatters wp-calypso#102523
5. i18n: Remove number formatting utilities/types from i18n-calypso wp-calypso#102878

[jsnajdr]

I upgraded to the fixed version of @knighted/duel, removed the no-longer-needed oxc-parser dependency, and implemented the feedback about build scripts and dependencies non-strict versioning. All CI checks are green and I believe we're ready for a final ✅ and shipping.

[chriskmnds]

Thanks again. Copied dist/ and package.json to number-formatters package in wp-calypso. yarn test-client and yarn typecheck pass with no errors now on wp-calypso side.

I'm seeing some TS warnings in JP, in my editor. They appear in several files, but these ones from get-cached-formatter.ts:

1. This only happens here, not in trunk (I'm guessing something to do with "target": "ES2024"):

2. This is also in trunk. I have tried to investigate but didn't get far. I'm guessing it's my environment/editor if you don't see them:

Code review obviously up to @anomiex

[jsnajdr]

I'm seeing some TS warnings in JP, in my editor.

This looks like a locally broken typescript installation. TypeScript cannot find its own libraries (lib/es2024.d.ts), ESLint cannot use typescript as resolver. I'd check the node_modules/typescript directory. Which version is installed there? Are there duplicate versions in packages/*/node_modules subfolders? Cleaning node_modules and reinstalling should help.

[anomiex]

Looks reasonable to me. No idea about @chriskmnds's errors.

[chriskmnds]

All good on my end. Refreshing dependencies and restarting the editor cleared these.

[chriskmnds]

p.s. If we wait for #42796 first (it's green) then we should be able to see this linked in the repo and if any issues on JP side. I think? However, I'm still trying fix things in my environment to point to a usage, so maybe the other way then. :)

[jsnajdr]

If we wait for #42796 first (it's green) then we should be able to see this linked in the repo and if any issues on JP side.

Sure, I don't mind waiting for this PR that switches usages from @automattic/format-currency (Calypso) to @automattic/number-formatters (Jetpack). But Jetpack won't "see" most changes introduced by the dual CJS/ESM build because it uses the jetpack:src field that points directly to the source. The dist folders are ignored in this setup.

[chriskmnds]

If we wait for #42796 first (it's green) then we should be able to see this linked in the repo and if any issues on JP side.

Sure, I don't mind waiting for this PR that switches usages from @automattic/format-currency (Calypso) to @automattic/number-formatters (Jetpack). But Jetpack won't "see" most changes introduced by the dual CJS/ESM build because it uses the jetpack:src field that points directly to the source. The dist folders are ignored in this setup.

Got it. Let's deploy this pls so we can make progress wth the wp-calypso PRs. I can do the 1.0.0 release if you like, unless you want to PCYsg-Np7-p2

I'm having an awful hard time getting my environment in place anyhow to test #42796, but like you said, it doesn't even matter for these changes.

[jsnajdr]

Let's deploy this pls so we can make progress wth the wp-calypso PRs. I can do the 1.0.0 release if you like

I merged this PR now, and let's release 1.0.0 only after it's actually verified in Calypso 👍