Skip to content

chore!: require Node.js 22 or newer#1747

Merged
arthurschreiber merged 5 commits into
masterfrom
chore/require-node-22
Jun 21, 2026
Merged

chore!: require Node.js 22 or newer#1747
arthurschreiber merged 5 commits into
masterfrom
chore/require-node-22

Conversation

@arthurschreiber

@arthurschreiber arthurschreiber commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Raises the minimum supported Node.js version to 22 and narrows CI to the currently supported release lines (22, 24, 26). This unlocks a dependency cleanup that was previously held back by the Node 18 baseline.

Warning

Breaking change. Node.js 22 is now the minimum supported version. Node.js 18, 20, and 21 are no longer supported.

Node version bump

  • engines.node: >=18.17>=22
  • @types/node range: >=18>=22
  • Babel preset-env target: node 18node 22
  • CI (nodejs.yml, appveyor.yml): test matrices → 22.x, 24.x, 26.x; single-version jobs (lint, Azure, pre-release) and release.yml → Node 22
  • devcontainer image → javascript-node:22

(27.x is intentionally omitted — Node 27 hasn't been released yet; it can be added once it ships.)

Dependency bumps

With the Node baseline raised:

  • Dropped the @azure/* overrides block — it existed only to hold the transitive Azure packages on their Node-18-compatible line (see chore(deps): update dependencies within existing semver ranges #1741). Removing it lets the Azure deps move to their latest, which now declare engines.node >=20.
  • Bumped direct Azure deps: @azure/core-auth ^1.10.1, @azure/identity ^4.13.1, @azure/keyvault-keys ^4.10.2
  • Bumped now-Node-gated devDeps: rimraf ^6.1.3, nyc ^18.0.0, semantic-release ^25.0.5
  • Removed the unused, deprecated codecov npm package — CI uploads coverage via codecov/codecov-action@v5, and nothing references the npm package.

Deliberately deferred (not Node-driven, would be breaking on their own):

  • eslint 10 — current @typescript-eslint 8.x only peers ^8 || ^9, so it would force a typescript-eslint v9 major
  • chai 5/6 — ESM-only, requires rewriting test imports
  • @types/lru-cache 7 — the code still uses the old import LRU from 'lru-cache' API

The remaining npm audit advisories (mocha → serialize-javascript/diff, nyc/babel-plugin-istanbul → js-yaml, and the undici bundled inside the npm CLI used by semantic-release) are all dev-only and will be looked at separately.

Verification

Ran on Node v22.23.0: npm install, npm run build, npm run lint (eslint + tsc), and all 394 unit tests pass.

🤖 Generated with Claude Code

Raise the minimum supported Node.js version to 22 and run CI only on
currently supported release lines (22, 24, 26).

- engines.node: >=18.17 -> >=22
- @types/node range: >=18 -> >=22
- Babel preset-env target: node 18 -> node 22
- CI (nodejs.yml, appveyor.yml): test matrices -> 22.x, 24.x, 26.x;
  single-version jobs and release.yml -> Node 22
- devcontainer image -> javascript-node:22

With the Node baseline raised, drop the @azure/* overrides block (added
only to hold the transitive Azure packages on their Node-18 line) and
bump the direct Azure deps to their latest, which now require Node >=20:
@azure/core-auth ^1.10.1, @azure/identity ^4.13.1,
@azure/keyvault-keys ^4.10.2. Also bump the now-Node-gated devDeps:
rimraf ^6.1.3, nyc ^18.0.0, semantic-release ^25.0.5.

Verified on Node v22.23.0: npm ci, build, lint (eslint + tsc), and all
394 unit tests pass.

BREAKING CHANGE: Node.js 22 is now the minimum supported version.
Node.js 18, 20, and 21 are no longer supported.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

This is a clean, well-scoped infrastructure PR. The PR description is thorough and the rationale (unblocking Azure dep cleanup, following Node's support lifecycle) is sound. Below are my observations.


Summary

The PR bumps the minimum Node.js requirement from 18 → 22, updates CI matrices (GitHub Actions + AppVeyor + devcontainer), raises direct Azure SDK deps to their latest Node-20-gated versions, removes the overrides block that held those deps on their Node-18-compatible lines, and bumps a few devDeps that were also Node-gated.


Observations

Potential Issue: engines.node vs actual semantic-release requirements

The package.json declares "engines": { "node": ">=22" }, but the upgraded @semantic-release/github@12 and @semantic-release/npm@13 both declare:

"engines": { "node": "^22.14.0 || >= 24.10.0" }

This means Node 22.0–22.13 satisfies the library's own engines.node constraint but would fail to run the release tooling. Since these are devDependencies used only in the release workflow and CI is pinned to 22.x (which resolves to the latest 22.x patch), this won't affect CI or consumers of the library directly. But a contributor who happens to be running an older Node 22 patch (e.g., 22.0 or 22.12) could hit unexpected failures.

Suggestion: Consider tightening the engines field to >=22.14.0 (or "^22.14.0 || >=24") to better reflect the actual floor imposed by the toolchain. Alternatively, a note in the contributing docs would help.


Minor: Node.js 20 dropped while still in Active LTS

Node.js 20 remains in Active LTS until April 2026 (Maintenance until April 2027). Dropping it is a valid and deliberate choice, but it's worth noting explicitly in the release notes/CHANGELOG — users on Node 20 will need to stay on the previous tedious release until they upgrade their runtime.

The PR description already marks this as a breaking change, which is correct. Just making sure the actual release changelog communicates this clearly.


Node.js 26 in CI matrix

Node 26 is included in the CI matrix (22.x, 24.x, 26.x), but as of the time of this PR it is the "Current" (non-LTS) release. This is pro-active and good practice for catching future-compatibility issues early. No concern here — just flagging it as intentional forward-testing rather than LTS coverage.


devcontainer was on Node 16, not 18

-    image: "mcr.microsoft.com/devcontainers/javascript-node:16"
+    image: "mcr.microsoft.com/devcontainers/javascript-node:22"

The devcontainer was pinned to Node 16 (two major versions behind even the old baseline of 18). Good catch and good fix, though it explains why the devcontainer may have been untested for a while. This is a two-version leap from the old engines.node minimum.


@types/node in dependencies (pre-existing, not introduced here)

Not introduced by this PR, but worth noting for maintainers: @types/node is listed under dependencies (not devDependencies), which means it will be installed for library consumers. Narrowing the range from >=18>=22 is consistent with the engines bump, but any consumer with @types/node@18 or @types/node@20 in their own project may see version-resolution churn. This is expected behavior for a major version bump.


Package-lock churn

The lockfile diff is large (+2057/-2157 lines) but is an expected cascade from the Azure SDK major bumps. Nothing concerning — the transitive updates all move toward newer versions with >=20 engine floors, consistent with the new baseline.

Notable transitions in transitive deps:

  • @azure/msal-node: v3 → v5 (drops uuid as a dep)
  • @azure/msal-browser: v4 → v5
  • @semantic-release/github: v9 → v12
  • @semantic-release/npm: v11 → v13
  • Several @octokit/* packages bumped to versions requiring >=20

What Looks Good

  • All six changed files are internally consistent — no mismatched version references found.
  • Babel target updated to node: 22 in package.json, correctly matching the engines bump.
  • overrides removal is clean — no leftover stale pins.
  • CI matrix covers 22 (current LTS), 24 (next LTS), and 26 (Current).
  • PR is appropriately scoped: deliberately defers eslint@10, chai@5/6, and @types/lru-cache@7 to avoid conflating a Node-floor bump with unrelated breaking changes.
  • 394 unit tests passing on Node 22 per the PR description.

Summary Verdict

Approve with minor suggestions. The implementation is correct and the scope is appropriately tight. The only actionable item worth considering before merge is whether to tighten engines.node to >=22.14.0 to match what the release tooling actually requires.

The `codecov` npm package is no longer used: CI uploads coverage via the
`codecov/codecov-action@v5` GitHub Action, and no script references the
package. It is also deprecated and unmaintained, and pulled a number of
vulnerable transitive dependencies (js-yaml, teeny-request, uuid,
@tootallnate/once, http-proxy-agent) into the dev dependency tree.

Verified on Node v22.23.0: lint (eslint + tsc) and all 394 unit tests
pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@arthurschreiber arthurschreiber force-pushed the chore/require-node-22 branch from 657ce0a to 95673f0 Compare June 21, 2026 16:45
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This is a clean, well-scoped maintenance PR that raises the minimum Node.js requirement from 18.17 to 22, updates the CI matrix to test on 22/24/26, and takes advantage of the raised baseline to bump Azure deps and address 12 of 13 npm audit advisories. The PR description is thorough and documents the reasoning and trade-offs well.


Positives

  • Excellent PR description. The summary documents each change category, explains motivations (e.g., the Azure overrides block removal), identifies the remaining advisory and why it can't be fixed, and calls out deliberately deferred work. This is the right level of detail for a breaking-change PR.
  • Security hygiene. Removing the unused codecov npm package and adding overrides for serialize-javascript, diff, and js-yaml are good moves. The PR correctly verifies API compatibility for the js-yaml 4.x override (.load() exists in 4.x).
  • CI matrix is appropriate. 22.x (LTS), 24.x (LTS), and 26.x (LTS since October 2025) cover all currently-supported active lines. Noting why 27.x is omitted is a nice touch.
  • No source file changes. The absence of any src/ or test/ modifications is reassuring — this is purely a tooling/config bump with no logic risk.

Issues & Observations

Minor

1. Devcontainer was on Node 16, not 18

The old devcontainer image was javascript-node:16, not 18, meaning it was already two major versions behind the previously-declared minimum. Not introduced by this PR, but worth noting so it doesn't silently drift again.

2. @types/node is in dependencies, not devDependencies

Type declaration packages should not be published as runtime dependencies — they have no effect at runtime and unnecessarily force the type package on consumers. This is a pre-existing issue, not introduced here, but would be a good cleanup in a follow-up PR.


Worth Discussing

3. @azure/msal-node 3.x → 5.x is a significant major version jump

The bump skips a full major version (3.8.105.2.5), and @azure/msal-browser also jumps from 4.x to 5.x. The PR notes all 394 unit tests pass, which is reassuring, but these are MSAL authentication libraries where major bumps can change token caching, authority handling, or credential flow behavior. It would be worth confirming:

  • Integration tests covering Azure AD auth paths (azure-ad-auth, azure-ad-service-principal-auth) pass against this branch.
  • There are no behavioral differences in how tokens are acquired or cached in real Azure SQL scenarios.

4. Implicit Node 22.14+ requirement for the release job

@semantic-release/github and @semantic-release/npm now declare an engine requirement of ^22.14.0 || >= 24.10.0. The release workflow uses node-version: "22" (resolves to the latest 22.x patch), so this works fine today. However, this is a latent footgun: pinning to a specific 22.x below 22.14 would cause confusing failures. Consider making the constraint explicit in release.yml with node-version: "22.14" or noting it in a comment.

5. Remaining advisory: undici via npm CLI

The PR correctly explains this is unfixable from outside npm. No action needed, but a note in SECURITY.md or a comment in the overrides block would help future contributors understand why the advisory is left standing.


Summary

This is a well-executed dependency maintenance PR with a clear rationale and no source code changes. The main items to confirm before merging:

  1. Confirm Azure AD integration test results on this branch — the msal-node 3→5 jump is the highest-risk change given it skips a major version in authentication-critical code.
  2. Consider pinning node-version in release.yml to 22.14 to surface the semantic-release engine constraint explicitly.
  3. The @types/node in dependencies (vs. devDependencies) is pre-existing and can be deferred to a follow-up.

🤖 Reviewed with Claude Code

@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This is a well-scoped, well-documented breaking-change PR that raises the minimum Node.js runtime to v22, updates the CI matrix to {22, 24, 26}, removes the legacy overrides block, and takes the opportunity to bump the Azure SDK deps and several devDependencies that were blocked by the old Node 18 baseline. The PR description clearly explains what's in scope and what's deliberately deferred.


Positive observations

  • Clean overrides removal — the @azure/* overrides block in package.json was a tech-debt workaround (see chore(deps): update dependencies within existing semver ranges #1741) that is now correctly eliminated now that all Azure deps have moved past their Node-18-only line.
  • Consistent updates — the node version target is changed everywhere: engines, @types/node, @babel/preset-env target, every CI job, the devcontainer image, and AppVeyor. Nothing was missed.
  • Devcontainer fixed — the devcontainer was pinned to Node 16 before this PR, so bumping it to 22 also closes a pre-existing inconsistency.
  • codecov npm package removed — correctly identified as unused since coverage is already uploaded via codecov/codecov-action@v5.
  • Node 26 included in matrix — appropriate as Node 26 shipped in April 2026 and is the current active release line.

Issues / suggestions

Minor: engines field may be too permissive for semantic-release workflows

The package engines field is set to "node": ">=22", but several pulled-in semantic-release plugins now declare "node": "^22.14.0 || >= 24.10.0":

  • @semantic-release/github@12
  • @semantic-release/npm@13

Because these are devDependencies this is invisible to library consumers, and CI runs against the latest 22.x which will satisfy ^22.14.0. However, a developer running exactly Node 22.0–22.13 could hit breakage when running semantic-release locally. The inconsistency is low-risk in practice, but it might be worth tightening to >=22.14.0 (matching the strictest constraint among the devDependency chain) — or at minimum noting it in CONTRIBUTING docs.

Nit: @types/node in dependencies (pre-existing, not introduced here)

@types/node is listed under dependencies rather than devDependencies. This means it gets installed for end-users of the package, which is unconventional for a types-only package. The range bump from >=18 to >=22 is correct given the new minimum, but the placement may be worth revisiting in a follow-up if it's not intentional.

Nit: AppVeyor uses npm install instead of npm ci (pre-existing)

appveyor.yml still uses npm install while all GitHub Actions jobs use npm ci. This is a pre-existing inconsistency, not introduced by this PR, but since the file is being touched it would be a good time to align them.


Security / dependency notes

  • The bumped @azure/msal-node (v3 → v5) drops the uuid dependency entirely — this is an upstream improvement that shrinks the dependency surface.
  • The remaining npm audit advisories called out in the PR description (mocha → serialize-javascript, nyc/babel-plugin-istanbul → js-yaml, undici inside the npm CLI) are all dev-only and correctly deferred.
  • No new prod dependencies are introduced; all additions are in the devDependency tree.

Summary

The PR is correct, complete, and safe to merge. The only non-trivial point worth a brief discussion before merging is whether "node": ">=22" in engines should be ">=22.14.0" to reflect the strictest constraint imposed by the devDependency chain (semantic-release plugins). All other findings are pre-existing or nits.

🤖 Reviewed with Claude Code

The "reads data that is sent across multiple packets, with a chunk
containing multiple packets" test asserted that three packet payloads
arrive merged into a single chunk ([ payload ]). That merging was never
a tedious behavior: it came from Node's Readable.read() (used by the
async iterator) concatenating the whole internal buffer when called
with no size argument.

Node commit fadb214d95 ("stream: readable read one buffer at a time",
nodejs/node#60441) changed howMuchToRead() to return one buffer at a
time, so the async iterator now yields each packet payload as its own
chunk. Node 24 still has the old behavior, Node 26 has the new one,
which is why only the Node 26 CI job failed.

How IncomingMessageStream chunks the payloads is an implementation
detail the test never cared about (the original comment said as much),
so assert on Buffer.concat(receivedData) instead of the exact chunk
boundaries. Verified on both Node v22.23.0 and v26.3.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This PR raises the minimum Node.js version from 18 to 22, narrows the CI matrix to currently-supported release lines (22, 24, 26), bumps direct Azure dependencies that were previously held back by the Node 18 baseline, and removes the now-unused codecov npm package. It is well-scoped and the PR description thoroughly explains the reasoning and intentional deferrals.


Positive Aspects

  • Clean, focused change — one concern per PR, clear commit message following conventional commits (chore!: for breaking change).
  • Excellent PR description — explicitly lists what was deferred (eslint 10, chai 5/6, @types/lru-cache 7) and the reason for each. This avoids future confusion.
  • Correct removal of the @azure/* overrides block — the overrides were a workaround for the old Node 18 baseline; removing them allows the Azure SDK chain to move to current versions.
  • codecov npm package removal — the package is deprecated and the repo already uses codecov/codecov-action@v5 in CI. Removing the unused npm package is the right call.
  • rimraf, nyc, semantic-release bumps are all appropriate for the new Node floor.

Observations & Suggestions

1. @semantic-release/github and @semantic-release/npm have a narrower engine constraint than >=22

Both new versions declare "engines": { "node": "^22.14.0 || >= 24.10.0" }. The release workflow pins node-version: "22", which resolves to the latest 22.x patch — as of today that is ≥22.14.0, so this is fine in practice. But it is worth knowing: if the CI image for any reason resolved an older 22.x patch (e.g. cached), the semantic-release run could fail at install time. No action required, just something to be aware of.

2. @azure/msal-node skipped a major version (3.x → 5.x)

The lock file shows @azure/msal-node jumped from 3.8.10 to 5.2.5 as a transitive dep of @azure/identity 4.13.1. The @azure/identity range itself only moved by a minor version (4.10.1 → 4.13.1), but that carried a transitive major bump inside. Since tedious interacts with msal-node only indirectly via @azure/identity's public API, this should be transparent — but it's worth a quick sanity-check against the @azure/identity 4.11–4.13 changelog to confirm no behavioral changes affect the auth flows tedious exercises.

3. @azure/msal-common also had a major bump (15.x → 16.x)

Related to the above: msal-common is shared between msal-browser and msal-node, so both jumped. Again, this is fully encapsulated behind @azure/identity's API surface, but noting it for completeness.

4. @types/node is in dependencies, not devDependencies (pre-existing)

@types/node: ">=22" lives in the dependencies block, not devDependencies. This means every consumer of the tedious package installs it at runtime. This is a pre-existing issue — not introduced by this PR — but bumping the range to >=22 makes it slightly more visible. If @types/node is needed at publish time for the .d.ts output, it could be pinned via peerDependencies or moved to devDependencies with a separate bundled types approach. Worth a follow-up ticket.

5. devcontainer image was at 16, not 18 (pre-existing gap, now corrected)

The docker-compose.yml jumped from :16 to :22. The previous image was two majors behind CI — that inconsistency is fixed by this PR, which is good.

6. Node 26 in AppVeyor

Node 26 was released in April 2025 and AppVeyor supports it, so adding it to the matrix is valid. Node 26 is not yet LTS (LTS expected October 2026) but testing against it proactively is reasonable and consistent with the GitHub Actions matrix.

7. New transitive devDependency: @actions/core

@semantic-release/npm 13.x now depends on @actions/core, which pulls in @actions/exec, @actions/http-client, and @actions/io. These are GitHub Actions SDK packages — harmless as devDeps, but slightly unusual to see them in a non-Actions-native project. This is upstream's choice, not a concern for this PR.


Verdict

Approve with minor notes. The change is correct and well-prepared. The observations above are either pre-existing issues or informational notes rather than blockers. The one thing worth a quick manual verification before merge is the @azure/identity 4.11–4.13 changelog to confirm the msal-node 3→5 transitive jump doesn't introduce any behavioral changes in the credential flows tedious tests in CI.

🤖 Generated with Claude Code

Update-NodeJsInstallation defaults to the x86 (32-bit) Windows build
when no architecture is given. Node.js no longer ships 32-bit Windows
builds for the supported release lines (22, 24, 26), so the install
step failed. Pass x64 explicitly to install the 64-bit build.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This PR is a well-scoped, infrastructure-only change that raises the Node.js minimum from 18 to 22, updates the CI matrix to [22.x, 24.x, 26.x], and unlocks dependency cleanup that was previously blocked by the Node 18 baseline. There are no production source-code changes.


Strengths

  • Excellent PR description. Clearly explains the rationale, what was bumped, what was deliberately deferred, and why. The "Deliberately deferred" section is particularly valuable for reviewers.
  • Clean Azure override removal. Dropping the overrides block added in chore(deps): update dependencies within existing semver ranges #1741 is the right follow-through once the baseline moves to 22.
  • Deprecated codecov package removed. The npm package has been deprecated since 2022 and CI already uses codecov/codecov-action, so this is a clean win.
  • AppVeyor x64 arch flag. Subtle but useful fix: Get-NodeJsLatestBuild defaulted to x86 on newer AppVeyor images; the explicit x64 arg prevents silent 32-bit installs.
  • devcontainer image jump (16 to 22). The container was already two LTS cycles behind; catching this is good housekeeping.
  • 394 unit tests passing on Node 22 is reassuring verification.

Observations and Suggestions

1. MSAL major version bumps are transitive but non-trivial

@azure/identity 4.13.1 pulls in @azure/msal-browser 5.x (was 4.x) and @azure/msal-node 5.x (was 3.x). Both are major bumps in Microsoft's auth library. This is transitive and unavoidable given the identity version bump, but it's worth confirming that Azure AD / service-principal integration tests still pass against these new MSAL versions — the PR description notes the 394 unit tests pass but does not specifically mention the Azure auth jobs.

2. Dev tooling engines vs. stated minimum

@semantic-release/github and @semantic-release/npm declare engines: "^22.14.0 || >= 24.10.0", while the package's own engines.node is >=22. This does not affect library users, but a developer running npm run release on Node 22.0-22.13 would get an engines warning (or failure with --engine-strict). Node 22.14.0 was the first 22.x LTS release (2025-02-11), so anyone on a current 22.x install is already past it. Low risk in practice, but worth noting in the changelog or docs.

3. Node 26 is "Current", not LTS

Including 26.x in the CI matrix is consistent with the old strategy of including one non-LTS release (the old matrix had 21.x). This is fine for early-compat signalling. The PR description already calls out the 27.x omission clearly. Just make sure there is a plan to swap it for a future version once 26 approaches EOL.

4. @types/node range

Updating to >=22 is correct. Downstream consumers who pin @types/node to an older major will see a peer conflict. Expected and acceptable for a documented breaking change, but worth calling out in release notes.

5. CI job naming

The fixed-version Azure auth jobs (e.g. azure-sql-auth) now show Node.js 22.x in their display name. If those jobs ever expand to a matrix, the hardcoded name will be misleading. As-is for a single-version job this is fine.


No Issues Found With

  • Babel preset-env target (node: 22) aligns with the new minimum.
  • rimraf ^6, nyc ^18, semantic-release ^25 are all correct latest-major bumps for Node 22+.
  • package-lock.json churn is large but expected; lockfile was regenerated cleanly.
  • CI fail-fast: false is preserved across all matrix jobs.

Summary

This is a clean, well-prepared infrastructure modernization. The main thing worth confirming before merging is that the Azure integration CI jobs (azure-sql-auth, azure-ad-auth, token-credential-auth, azure-ad-service-principal-auth) pass with the new MSAL 5.x transitive deps — either by waiting for CI to run green or by referencing prior passing runs on the branch. Everything else looks correct and well-reasoned.

Verdict: Approve pending Azure CI green.

Node.js 22+ refuses to run on Windows older than Windows 10 / Windows
Server 2016 (exit code 216). AppVeyor's default image, Visual Studio
2015, runs on Windows Server 2012 R2, so the build failed as soon as
node was invoked.

Pin the Visual Studio 2022 image, which runs on Windows Server 2022.
SQL Server 2017 is still pre-installed there, so the existing
`mssql2017` service is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This is a well-scoped Node.js version bump that raises the minimum from 18.17 to 22, updates CI matrices to cover 22/24/26, and unlocks a dependency cleanup previously blocked by the Node 18 baseline. The PR is clean and purposeful, with a helpful description of what was deferred and why.


Strengths

  • Focused scope: No unrelated changes. The diff is exactly what a version bump chore should be.
  • AppVeyor comment is excellent: The explanation of why Visual Studio 2019 is required (Node 24+ refuses to start on Windows Server 2016) is a non-obvious constraint that would be painful to rediscover. Documenting it at the right layer is great.
  • Test fix is correct: The message-io-test.ts change (assert.deepEqual(Buffer.concat(receivedData), payload)) correctly identifies that stream buffering behavior is an implementation detail that varies across Node versions. The concat-and-compare approach is more robust and future-proof.
  • overrides removal: Dropping the Azure-specific overrides block is the right call now that the Node 22 floor is set.
  • codecov removal: Good cleanup — the package was deprecated and CI already uploads coverage via codecov/codecov-action@v5.

Issues to Address

1. AppVeyor Windows CI blind spot (medium)

The comment in appveyor.yml notes:

for pull request builds AppVeyor resolves image from the base branch, so this only takes effect once it is on the default branch

This means Windows CI for Node 24 and 26 cannot be validated by this PR itself — it will only take effect post-merge. If there is a Windows-specific failure on Node 24/26, it will only surface after the fact. Worth confirming whether a post-merge smoke run is planned before the next release.

2. MSAL major version jumps via @azure/identity (low-medium)

The transitive deps @azure/msal-browser and @azure/msal-node jumped two major versions each:

  • msal-browser: 4.x → 5.x
  • msal-node: 3.x → 5.x

These are indirect (pulled in by @azure/identity), so tedious doesn't call them directly. But if there are behavioral changes in token acquisition, error shapes, or credential flows in those majors, they could silently affect the Azure auth integration tests. The PR body doesn't mention whether the Azure CI suite was exercised — worth confirming the azure-sql-auth, azure-ad-auth, token-credential-auth, and azure-ad-service-principal-auth jobs ran green.

3. engines.node range vs. semantic-release requirements (cosmetic)

package.json declares engines.node: >=22, but the updated @semantic-release/npm@13.1.5 and @semantic-release/github@12.0.8 both require ^22.14.0 || >= 24.10.0. So the project could theoretically be installed and run on Node 22.0–22.13, but semantic-release would fail at release time.

This is a dev-only concern (CI will be fine), so severity is low. If you want precision, bumping engines.node to >=22.14 would reflect the true minimum for the full toolchain. Otherwise a note in the changelog is sufficient.


Minor Observations

  • The old matrix included Node 21 (odd, non-LTS, now EOL). The new matrix includes Node 26 (odd, current). This is consistent with the project's apparent policy of testing against the current release alongside LTS lines — looks intentional.
  • The note "27.x is intentionally omitted — Node 27 hasn't been released yet" is accurate and helpful context.
  • The x64 flag added to the AppVeyor PowerShell install command is a good defensive addition for newer Node versions.
  • @azure/msal-node v5 dropped uuid as a dependency — Node 22 ships crypto.randomUUID() natively, so this is a clean improvement.
  • The devcontainer was on Node 16 (not 18), so this corrects an existing gap while upgrading.

Test Coverage

No new tests are needed for a dependency/version bump. The one test change (stream buffering assertion) is appropriate and well-explained.


Summary

Solid maintenance PR. Two things worth confirming before merge:

  1. The Azure auth CI jobs ran green (to catch any MSAL behavior changes from the indirect major version bumps).
  2. A plan exists for validating Windows Node 24/26 behavior post-merge, given the AppVeyor PR-build limitation.

Everything else is correct or cosmetic.

Reviewed by Claude Code

@arthurschreiber arthurschreiber force-pushed the chore/require-node-22 branch from 5569162 to dfe0df0 Compare June 21, 2026 22:16
@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review: chore!: require Node.js 22 or newer

Overview

This is a clean, well-scoped maintenance PR that bumps the minimum Node.js version from 18.17 to 22, narrows the CI test matrix to currently-supported release lines (22, 24, 26), removes the now-unneeded @azure/* overrides block, and brings several devDependencies up to their latest Node-gated versions. The PR description is thorough and honestly documents what was deferred and why.


Positives

  • Deprecated codecov removal is a good security/maintenance win. The package was deprecated and flagged; CI already uses codecov/codecov-action@v5 instead.
  • AppVeyor image upgrade (Visual Studio 2015Visual Studio 2022) is necessary — newer Node requires Windows Server 2016+. The inline comment explaining why the image was pinned is helpful. The added explicit x64 flag to Update-NodeJsInstallation prevents silent fallback to 32-bit Node on some AppVeyor environments.
  • Azure overrides removal correctly cleans up the workaround from chore(deps): update dependencies within existing semver ranges #1741 now that the underlying blocker (Node 18 minimum) is gone.
  • Babel preset-env target updated consistently with the engine change.
  • Devcontainer image finally bumped from javascript-node:16 to 22 (was three major versions behind).

Issues / Concerns

1. engines.node is slightly looser than the actual minimum (minor)

package.json declares:

"engines": { "node": ">=22" }

But two of the newly-pulled devDependencies declare a tighter engine requirement:

  • @semantic-release/github@12: "node": "^22.14.0 || >= 24.10.0"
  • @semantic-release/npm@13: "node": "^22.14.0 || >= 24.10.0"

These are devDeps used only in the release workflow, so this won't affect library consumers. But running npm run release on Node 22.0.0–22.13.x would silently break. Consider tightening to >=22.14.0 (or documenting the semantic-release constraint) to avoid a confusing failure for contributors who happen to be on an earlier Node 22 patch.

The PR author tested on v22.23.0, which satisfies the stricter requirement — so this is working fine in practice, but the declared engine range is misleading.

2. Test change in message-io-test.ts — verify the intent (minor)

- assert.deepEqual(receivedData, [ payload ]);
+ assert.deepEqual(Buffer.concat(receivedData), payload);

This is a good fix — the old assertion assumed all data arrives in exactly one chunk, which apparently varies across Node.js versions. The new version is agnostic to chunking and only asserts on the concatenated payload. The updated comment explains the rationale well.

One small nit: the original test was implicitly also checking that exactly one chunk arrived. If preserving that coverage matters, a secondary assert.ok(receivedData.length >= 1) could be added — though the comment explicitly says we don't care about chunking, so dropping that check is fine.

3. @types/node in dependencies (pre-existing, informational)

The @types/node bump from >=18>=22 is correct, but this package remains in dependencies (not devDependencies). TypeScript consumers of tedious will now transitively require @types/node >=22. This is appropriate given the engine requirement, but worth calling out for downstream users migrating from Node 18/20.


No Issues Found

  • Removal of the five @azure/* overrides entries is safe — each is correctly traced back to the Node-18 compat constraint.
  • @azure/msal-node v5 now requires >=20; this is fine since tedious itself now requires >=22.
  • The remaining npm audit advisories (dev-only, mocha/nyc/undici-in-npm-cli) are correctly deferred and acknowledged.
  • Node 26 in the CI matrix: Node 26 was released in April 2026, so including it now is appropriate forward-looking coverage.

Summary

Approve with one suggested follow-up: tighten engines.node to >=22.14.0 to accurately reflect the real minimum imposed by the semantic-release plugins, or add a note to the contributing docs. Everything else looks correct and well-reasoned. The test fix in message-io-test.ts is a nice Node-version-robustness improvement.

🤖 Generated with Claude Code

@arthurschreiber arthurschreiber merged commit da2375a into master Jun 21, 2026
29 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 20.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant