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.