build: replace Babel with type-stripping (tsc) and modernize toolchain

[arthurschreiber]

Babel was doing little beyond stripping types: the @babel/preset-env
downleveling is effectively a no-op at the Node target, and the only
runtime-emitting TypeScript feature in the codebase was a handful of
enums. This switches the build to a pure type-stripping approach.

• Convert the four enums in always-encrypted/types.ts to as const
  objects (plus type aliases where the name is used as a type), so the
  source no longer relies on TypeScript-only runtime syntax.
• Build JavaScript with tsc via the new tsconfig.build.json (CommonJS
  output + source maps). Declarations are still emitted via
  tsconfig.build-types.json.
• Drop the redundant CommonJS module.exports re-exports, leaving the
  source as clean ESM.
• Run tests through tsx as the Mocha loader instead of
  @babel/register. tsx works directly now that the mixed
  module.exports/ESM pattern is gone, and it needs no compiler
  configuration.
• Collect coverage with c8 (V8 native) instead of nyc +
  babel-plugin-istanbul.
• Allow the const + type declaration-merge pattern via
  @typescript-eslint/no-redeclare.
• Raise the Node floor to the oldest supported release line (>=22) and
  update CI to test on Node 22/24/26.

https://claude.ai/code/session_019NDt99jpfPSgh4VPnTW3kK

[claude]

Code Review: Replace Babel with type-stripping (tsc) and modernize toolchain

This is a well-motivated, coherent change. Removing Babel simplifies the dependency tree significantly (~4331 deletions) and aligns the build with how the TypeScript compiler itself understands the code. Overall the approach is sound. A few items worth discussing:

---

Correctness / Potential Issues

1. PR description says ts-node but implementation uses tsx

The description states: "Run tests through ts-node/register (transpile-only, CommonJS)" but the actual change in package.json adds tsx as the devDependency and uses "node-option": ["import=tsx"] in the mocha config. tsx is esbuild-based and has different behaviour from ts-node (e.g. around decorator metadata, type-only imports, etc.). It's the right tool here, but the description is misleading. Worth updating before merge.

2. target: "esnext" + module: "commonjs" in tsconfig.build.json

"target": "esnext",
"module": "commonjs"

The in-file comment says "Strip types only - keep the modern syntax as-is", but tsc is doing more than type-stripping here: it's also converting import/export to require/module.exports. That's not really "strip types only" — it's still a syntactic transformation on module syntax. The comment is accurate in spirit (no JS downlevelling) but slightly misleading. A minor wording nit.

With Node ≥ 22 as the new floor this is perfectly fine in practice, since Node 22 ships V8 with full ESNext support.

3. DescribeParameterEncryptionResultSet1 / DescribeParameterEncryptionResultSet2 — no companion type alias

SQLServerEncryptionType and SQLServerStatementColumnEncryptionSetting each get a type alias so callers can still write the type name:

export type SQLServerEncryptionType = typeof SQLServerEncryptionType[keyof typeof SQLServerEncryptionType];

DescribeParameterEncryptionResultSet1 and DescribeParameterEncryptionResultSet2 don't get this treatment. If those names are never used as a type (only as value namespaces) that's fine — but it's worth double-checking that no .d.ts or consuming code references them as a type, or this is an unintentional regression in the public types surface.

4. isolatedModules: false in tsconfig.build.json is redundant

verbatimModuleSyntax: false already implicitly relaxes the isolated-modules constraint. Setting isolatedModules: false is harmless but adds noise alongside the comment. Could be dropped for clarity.

---

Breaking Changes

5. Dropping Node 18 and 20 support

"engines": { "node": ">=22" } is a potentially breaking change for users still on Node 18 LTS (which is supported until April 2025) or Node 20 LTS (supported until April 2026). Node 20 LTS is still in active maintenance as of this PR. This should be called out clearly in the release notes / CHANGELOG, and ideally gated behind a semver-major bump.

6. Node 26 in CI matrix

Node 26 was released in April 2025 but is not yet LTS (scheduled October 2025). Testing against it is forward-looking and good practice, but it may introduce unexpected CI flakiness from unstable APIs. Consider marking it continue-on-error: true until it hits LTS, consistent with how many projects handle "current" Node releases.

---

Positive Highlights

• c8 over nyc + babel-plugin-istanbul: V8-native coverage is more accurate and doesn't require source instrumentation. Correct choice given the build change.
• Enum → as const conversion: The pattern of re-exporting the same name as both a const and a type (with the ESLint no-redeclare exemption) correctly mirrors the original enum semantics for both runtime and type usage.
• Removing module.exports = ... dual assignments: These were a legacy compatibility shim that made little sense once the build output is controlled by tsc --module commonjs. Cleaning them up removes a long-standing inconsistency.
• Coverage exclusions in c8 config: Correctly excluding test/** and lib/** from coverage collection.
• tsconfig.build.json correctly disables verbatimModuleSyntax: This is necessary for --module commonjs and the comment explains exactly why. Good inline documentation.

---

Minor Nits

• The eslint-disable-next-line comments in always-encrypted/types.ts are clear but their explanatory text ("The value and the type intentionally share a name…") is slightly verbose for a disable comment. The rule name alone (@typescript-eslint/no-redeclare) and a short // const+type enum mimic would be sufficient.
• The c8 config in package.json uses extension (array) without include — consider also setting "include": ["src/**"] to be explicit about what's in scope, rather than relying on the implicit exclude list alone.

---

Summary: The change is technically solid and the direction is clearly correct — Babel was doing nearly nothing useful here. The main items to address before merge are: (1) updating the PR description to say tsx instead of ts-node, (2) explicitly calling out the Node 18/20 support drop as a breaking change in the release notes, and (3) verifying the missing type aliases for the two DescribeParameterEncryptionResultSet* constants.

[codecov]

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.21%. Comparing base (da2375a) to head (9f648b2).

Files with missing lines	Patch %	Lines
src/always-encrypted/types.ts	97.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1740       +/-   ##
===========================================
+ Coverage   79.98%   90.21%   +10.23%     
===========================================
  Files          90       93        +3     
  Lines        4901    15995    +11094     
  Branches      923     1546      +623     
===========================================
+ Hits         3920    14430    +10510     
- Misses        678     1552      +874     
+ Partials      303       13      -290     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review: Replace Babel with type-stripping (tsc) and modernize toolchain

This is a well-motivated toolchain cleanup. The core insight is sound. Babel was adding complexity (50+ transitive packages) to solve a problem that did not really exist at modern Node targets.

---

Overview

The PR replaces Babel + nyc with a lighter stack:

• Build: Babel to tsc via tsconfig.build.json
• Tests: @babel/register to tsx via --import=tsx in mocha config
• Coverage: nyc + babel-plugin-istanbul to c8 (V8-native)
• Runtime enums: four TypeScript enums converted to as const objects
• Dual-export cleanup: removes module.exports lines that were Babel-specific workarounds

The direction is correct and the execution is mostly clean.

---

Concerns

1. PR description says ts-node, but the implementation uses tsx

The PR body says "Run tests through ts-node/register (transpile-only, CommonJS)" but the actual implementation adds tsx as the dev dependency and uses node-option: import=tsx in the mocha config. tsx uses esbuild under the hood, not the TypeScript compiler, while ts-node uses the actual TS compiler API. The PR description should be corrected.

For this codebase (no decorators, no complex emit features), tsx is a perfectly fine choice -- faster and lower-overhead than ts-node. But it does mean tests compile via esbuild while the published build uses tsc, which is a subtle semantic gap. This is the same trade-off the project already accepted with Babel, so not a regression.

2. Node 22 is dropped from CI despite engines.node >=22

All CI jobs now run Node 24, and the Azure-specific jobs are renamed to reflect this. But the engines field still advertises Node 22 support. Users on Node 22 will not have any CI coverage. Either keep a Node 22 job in the matrix to validate the stated floor, or bump engines.node to >=24 if Node 22 is being intentionally dropped.

The PR description says "update CI to test on Node 22/24/26" but the diff only shows changes to 24 for the single-version jobs. If there is a matrix that still includes 22, that resolves this concern.

3. module.exports removal is a behavioral change for CJS consumers of internal paths

Files like src/bulk-load.ts previously had both export default BulkLoad and module.exports = BulkLoad. With Babel, the module.exports override meant require('./lib/bulk-load') returned BulkLoad directly. After this PR, that returns { default: BulkLoad, __esModule: true }. If any consumer does deep-requires into lib/ paths (unlikely but possible), they would break silently. The public API (lib/tedious.js) is fine, and the removal is correct for a tsc build -- just worth noting in the PR description as a technically-breaking change for non-standard usage.

---

Positive observations

Enum-to-const conversion is well-executed. The explicit ordinal values in the resulting objects (e.g., KeyOrdinal: 0, DbId: 1) are actually more readable than implicit TypeScript enum numbering. The as const + type alias pattern correctly preserves type semantics for SQLServerEncryptionType and SQLServerStatementColumnEncryptionSetting. Not adding type aliases for DescribeParameterEncryptionResultSet1/2 is appropriate given they are only used as value lookups.

tsconfig.build.json is correctly structured. Disabling verbatimModuleSyntax for the CommonJS build (while keeping it on in the base tsconfig.json for type-checking) is exactly the right approach. target: esnext + module: commonjs means tsc strips types but preserves modern syntax, which is correct for Node 22+.

c8 is a meaningful upgrade. V8-native coverage does not require Babel instrumentation middleware, gives more accurate branch/line counts, and works correctly with TypeScript source maps. The exclusion of test and lib in the c8 config is appropriate.

Dependency footprint reduction is significant. Removing six Babel packages plus nyc and babel-plugin-istanbul eliminates hundreds of transitive dependencies. The package-lock.json shrinks by roughly 5,400 lines.

---

Minor nits

• The comment in tsconfig.build.json says "Strip types only -- keep the modern syntax as-is", which is slightly misleading since module: commonjs will also downlevel ES module syntax (import/export) to CJS (require/module.exports). Rephrasing to "preserve modern non-module syntax as-is" would be more precise.
• isolatedModules: false in tsconfig.build.json is explicitly set but is already the default when verbatimModuleSyntax is off -- not wrong, just redundant.
• The coverage/ addition to .gitignore is correct and needed.

---

Summary

This is a solid modernization PR with a well-reasoned rationale. The main actionable items are:

1. Fix the PR description to say tsx rather than ts-node/register.
2. Clarify Node 22 CI coverage -- either add it back to the matrix or update engines.node.
3. Note the module.exports removal as a minor breaking change for deep-require users.

[claude]

Code Review: Replace Babel with type-stripping (tsc) and modernize toolchain

This is a well-scoped toolchain modernization. The direction is correct and the implementation is clean. A few items worth discussing before merging.

---

Overview

This PR replaces Babel with native tsc for the build, switches test transpilation to tsx, replaces nyc+babel-plugin-istanbul with c8, converts 4 TypeScript enums to as const objects, removes the redundant dual export default / module.exports = pattern, and raises the Node floor to 22. The dependency reduction (~6 Babel packages + nyc removed) and the simplified pipeline are genuine wins.

---

Potential Issues

1. Breaking CJS deep-require interop (high priority)

Every source file previously had the pattern:

export default Connection;
module.exports = Connection;   // ← removed by this PR

With Babel, the trailing module.exports = X completely overrode the module's exports object, so require('tedious/lib/connection') returned Connection directly. With tsc emitting exports.default = Connection and no module.exports override, that same require now returns { default: Connection, __esModule: true }.

Internal imports are fine — tsc inserts __importDefault helpers that know to look at .default — but any downstream consumer doing a direct deep-require like require('tedious/lib/connection') or require('tedious/lib/data-types/bigint') will silently get the wrong object. Worth confirming whether this is a supported (or documented-as-unsupported) access pattern before landing.

2. Node >=22 is a breaking change

Dropping support for Node 18 and 20 is a legitimate direction, but it is a semver-major breaking change. The PR has no version bump or changelog entry, and the CI matrix silently removed older versions. If this is intentional, it should be called out in the release notes and the commit should be tagged accordingly. If the intent is to ship this as a patch/minor on an existing major, the Node floor change needs its own major bump first.

3. DescribeParameterEncryptionResultSet1 / DescribeParameterEncryptionResultSet2 lack type aliases

SQLServerEncryptionType and SQLServerStatementColumnEncryptionSetting get the export type Foo = typeof Foo[keyof typeof Foo] alias that mirrors the original enum's type usage. The other two resultset enums do not. If any external consumer or internal code currently uses these names as TypeScript types (e.g., let x: DescribeParameterEncryptionResultSet1), the build will now fail silently. A quick grep -r 'DescribeParameterEncryption' src would confirm whether the omission is safe.

---

Minor Notes

• PR description vs. implementation: The description says the test runner switches to "ts-node/register" but the actual package.json mocha config uses "import=tsx". Not a bug, but the description is inaccurate.

• tsconfig.build.json uses JSONC (comments in JSON): tsc supports this natively, so it works. Just worth noting for any JSON tooling (e.g., jq, schema validators) that may choke on the comments.

• target: "esnext" in tsconfig.build.json: The comment says "strip types only — keep the modern syntax as-is". This is correct and intentional given the Node 22 floor, but worth a brief CHANGELOG note so maintainers later don't wonder why async/await, optional chaining, etc. are emitted verbatim rather than downleveled.

• Explicit ordinal values in resultset enums: The original enums relied on TypeScript's auto-increment (0, 1, 2...). The as const equivalents now list values explicitly, which is more readable and removes the implicit ordering dependency. Good change.

---

Positives

• Eliminating 6 Babel devDependencies simplifies the dependency graph meaningfully.
• c8 (V8 native coverage) is more accurate than Babel-instrumented coverage and requires no source transforms at coverage time.
• tsx is significantly faster than @babel/register for test startup.
• Build and type-check now use the same tool (tsc), removing the risk of Babel and tsc diverging on their understanding of the code.
• The as const + type alias pattern is idiomatic modern TypeScript and avoids enum's runtime overhead.
• ESLint change ('no-redeclare': 'off' + '@typescript-eslint/no-redeclare': 'error') is the correct way to allow const+type name merging.

---

Summary

The build architecture change and enum migration are solid. The two items I would resolve before merging: confirm the deep-require CJS interop story (or explicitly document it as unsupported), and make sure the Node 22 floor is shipped as a major-version bump (or that it is intentionally landing on a new major already in flight).

[claude]

Code Review: Replace Babel with type-stripping (tsc) and modernize toolchain

This is a clean and well-motivated toolchain simplification. Removing Babel eliminates ~5 layers of transformation (preset-env, preset-typescript, register hook, babel-plugin-istanbul) in favor of tools that are closer to the metal. Here are my observations:

---

What's Well Done

• Rationale is accurate: @babel/preset-env at target: node22 was a near-no-op, and the only TypeScript runtime feature in use was enum. The PR correctly identifies and eliminates both.
• c8 is the right call: V8 native coverage is more accurate than Babel-instrumented coverage for async/generator-heavy code like this driver.
• ESLint rule swap (no-redeclare to @typescript-eslint/no-redeclare): Correct — the TS-aware version understands the const/type declaration-merge pattern and won't flag it.
• module.exports cleanup: Removing the redundant CJS dual-exports is the right call now that tsc handles the CJS wrapping in the build step.
• Benchmark fixes: Correctly updated require(...).default after the dual-export was removed.

---

Issues / Things to Verify

1. Missing type aliases for two enums

DescribeParameterEncryptionResultSet1 and DescribeParameterEncryptionResultSet2 were converted to as const objects but did not get a companion type alias (unlike SQLServerEncryptionType and SQLServerStatementColumnEncryptionSetting). This is fine if those objects are only ever used as value namespaces (e.g. DescribeParameterEncryptionResultSet1.KeyOrdinal), but if any function parameter or variable is typed as DescribeParameterEncryptionResultSet1 anywhere in the codebase, TypeScript will now error. Worth a quick grep -r 'DescribeParameterEncryptionResultSet' src to confirm they're only used as value lookups.

2. Public API break for direct internal imports

Removing module.exports = X changes the shape of compiled output for anyone doing:

const Foo = require('tedious/lib/some-module');  // was X, now { default: X }

The benchmarks were correctly patched, but this affects any consumers using internal paths directly. Worth a callout in the changelog/release notes even if this is technically unsupported usage.

3. Two-pass tsc build

The build runs tsc twice — once via tsconfig.build.json (JS + source maps) and once via tsconfig.build-types.json (declarations only). You could unify these by adding "declaration": true and "declarationDir": "lib" to tsconfig.build.json and dropping the second invocation. Not a blocker, just a minor efficiency note.

4. target: esnext in the published CJS output

tsconfig.build.json sets "target": "esnext" with "module": "commonjs". This means the published lib/ files will contain CJS wrappers (require/exports) around fully-modern JS (optional chaining, nullish coalescing, etc.). That is correct for Node >=22, but worth documenting explicitly so future contributors do not assume the lib output is downlevelled.

5. Node >=22 floor is a breaking change

Raising the engines floor drops Node 18 (EOL April 2025) and Node 20 (EOL April 2026). This is defensible timing, but it is a semver-major change for any consumer pinned to Node 20. Ensure the release notes flag this prominently.

6. Node 26 in the CI matrix

The matrix includes 26.x. As of mid-2026 Node 26 is still in "Current" (non-LTS) status. Tests may be flakier here due to API churn. Consider adding continue-on-error: true for Node 26 until it enters LTS, or at minimum add a comment in the workflow explaining the intent.

---

Minor Observations

• The eslint-disable-next-line + explanatory comment pattern around the const/type redeclarations is the right approach. Alternatively, since '@typescript-eslint/no-redeclare': 'error' is now set globally, you could configure ignoreDeclarationMerge: true in the rule options (if supported) to avoid the per-site suppression.
• The tsconfig.build.json exclude: ["src/**/*.d.ts"] correctly prevents ambient declaration files from being re-emitted.
• Deleting test/setup.js is clean — the tsx loader via node-option is a drop-in replacement with no config needed.
• Adding coverage/ to .gitignore is the right housekeeping; c8 writes there by default.

---

Summary

Solid modernization PR. The main things to double-check before merging are (1) the missing type aliases for the two DescribeParameterEncryptionResultSet enums, and (2) that internal-path consumers and the Node >=22 floor are called out in the changelog. Everything else is either correct or a minor improvement opportunity.