Draft
Conversation
0d85478 to
ebb5414
Compare
6c82d06 to
2c6fc71
Compare
155ae03 to
2e90025
Compare
7e5a6d7 to
042ab0b
Compare
Node.js 20 goes end-of-life in April 2026 so this will form part of the next major versions of our packages.
This means we're running with the latest versions of the Node.js majors that we support. I also switched our publish workflow to run on Node.js 24 rather than 22 - 24 is now the latest LTS version and 22 is in maintenance.
Node.js 22.12 removes the need for the `--experimental-require-module` CLI flag when importing native ESM into CommonJS contexts. We'd like to migrate Reliability Kit to be ESM and we'd like for the teams who use it to not have to include this CLI flag. We're taking the decision to drop support for Node.js versions below 22.12 in order to support this. This is the first step towards us migrating to native ESM, see #1479.
This will allow us to use conditional exports when we migrate to native ESM. It's also the more modern way of doing package exports as it's a lot more flexible than the top-level `main` property. Partially as a result of this change and partially because we'll be switching to TypeScript, I've also outlined some more strict requirements for Reliability Kit's usage with TypeScript, and the settings we recommend and support.
This migrates serialize-error and serialize request packages to use the Node.js test runner instead of Jest. This halves the time it takes to run these tests and brings us a step closer to getting rid of Jest.
This was simpler than I thought. We need a lot of command line arguments to actually run the tests (experimental mock modules, snapshots, and code coverage) but I think that's fine - they're all relatively stable and mean we can test with zero dependencies. Test runs drop from ~250ms to ~70ms.
This is probably the biggest suite of tests to migrate. It wasn't _easy_ but it was less of a pain than I thought. The Node.js test runner caters for everything we need. The only issue at the mo, as stated before, is that we need a lot of CLI arguments to enable all the testing features we need. These will slowly go away so I'm not worried. Test runs didn't speed up for this because the bulk of the time we spend is waiting for child processes in the end-to-end tests.
Nice and easy. Test runs go from ~200ms to ~70ms.
These tests aren't giving us anything because TypeScript will already have validated that our exported object is a valid config file. This brings the eslint-config package in line with the biome-config package.
This one's relatively straightforward. Test run drops from ~150ms to ~65ms.
Nice and easy. Run drops from ~120ms to ~50ms.
Test run time drops from ~2,100ms to ~1,800ms
Test runs from from ~125ms to ~60ms.
Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
These were not actually testing anything previously, and now we're being more explicit about which error properties we test. This fixes the snapshot tests.
This is a code smell. If you need to specify which files need coverage then you're not mocking enough. So I've removed all the easy ones. Part of my strategy for getting rid of these was to stop covering end-to-end tests, which shouldn't be mocking anything. The unit tests should provide 100% coverage and the end-to-end tests should be an added layer of safety on top of these and should not rely on mocking. This involved me splitting out the test commands, which is useful anyway because it means we can run the unit tests alone much quicker - the end-to-end tests are normally the slow ones. One test remains that needs the "include" flag, we should aim to remove this and mock things properly one day but let's not make too many more changes as part of this initial work.
This requires a bit of a refactor due to the way mocking works with ESM, and I've moved all the calculation of app info properties into a new function. It was annoying to have to do it, but I think the codebase and tests are a lot cleaner now. One thing to note is that we can no longer reliably import a JSON file so I've had to switch to `fs.readFileSync` - this shouldn't have a performance impact really as it'll be cached after the first run (just like a JSON `require`)
This is a large change because there are a lot of files. One thing that I was forced on was installing the prettifier by default and tweaking the way we check for it. I think it may be worth disabling the prettifier by default in a future breaking change so that we don't end up with prettified logs in production.
We're also removing the Sucrase tests as part of this change because `import.meta.dirname` doesn't work as expected. I don't think we should be encouraging Sucrase use now that Node.js has native ESM and TypeScript support so I'm happy to say we don't officially support it.
I had to switch the exclusions in the unit test function because Node.js test mocking behaves differently for ESM. It's a little gross but we could solve it by properly mocking the `errors` module. I didn't want to create too much churn in this PR though, so I'm OK with it for now.
042ab0b to
d277230
Compare
This involves a bit of a refactor because we can no longer clear the module cache. Once again it results in more sensible code and better tests 👍
This is a potential breaking change that we didn't document and might trip some people up if they're still using CommonJS. This is only needed for the packages where we still have a default export, many of them don't any more.
Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
This means packages can have their own build steps, allowing us to migrate them to TypeScript one-by-one instead of as one massive PR. See-also: https://financialtimes.atlassian.net/browse/CPREL-1481
There was no need to ever really use `jsconfig.json`, migrating it to `tsconfig.json` and configuring it to be a little more strict. This also updates all our documentation and adds the appropriate build scripts to allow us to build all the packages quickly _and_ watch for changes in local development.
There are a few key decisions in migrating this package that will apply
to all others:
1. We separate type checking and building. The type checking is done
via `tsconfig.json` and it will not emit any built files. This is
used to quickly validate that our types are correct as part of the
`lint` script. Splitting this out also means that we get type
checking for all files in the repo instead of just the files inside
the published packages.
2. We build with a new `tsconfig.build.json` file that uses
"references" to point to the different packages that we build with
TypeScript. This allows us to run a central `build` script and
it'll only include the packages that we've rewritten as TypeScript.
This also brings a lot of speed benefits and simplifies file
watching.
3. We add a `build:watch` script. Running the build is now essential
in local development, otherwise TypeScript won't be able to find
types in a dependent package (e.g. logger will get TypeScript
errors because the types for serialize-error can't be found). This
script makes it as easy as possible.
4. Tests are still run against the TypeScript files rather than the
built JavaScript files. This keeps the tests super fast and means
we don't have to build them, speeding up the TypeScript build. This
might be controversial because we're not testing the exact files we
publish but I'm OK with it for now - if we introduce a bug due to
this decision we can always reverse it.
5. We drop the `.npmignore` file in favour of specifying `files` in
the package manifests. This allows us to easily say "just publish
the dist folder" rather than having to match all the source "ts"
files in an npmignore. The only file we explicitly exclude is the
build info file, because it's meaningless in a published package.
The only breaking change that impacts this project is that types are not imported from `@types` automatically, so we need to specify them in the root TypeScript config.
I missed this requirement when working with source maps: in order for source maps to work correctly, the source file needs to be published alongside it otherwise the lines in the source map don't point to anything. So this ensures each TypeScript package so far is published _with_ TypeScript source files but still excludes the tests.
This also makes it much nicer, similar to Camille's script for creating Client Metrics namespaces. Co-Authored-By: Camille Croci <camillecroci@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a long-running branch to track our 2026 milestone. Each commit has been reviewed as part of a separate PR:
<=22.11#1626