fix(esm): emit Node-resolvable ESM by switching to nodenext#44
Conversation
Compiled output under `lib/` previously contained extensionless relative
imports (e.g. `import { Client } from './client'`), because tsconfig
used `moduleResolution: bundler`. Pure-Node ESM consumers hit
`ERR_MODULE_NOT_FOUND` since Node's ESM resolver requires explicit
file extensions; only bundler-based consumers worked.
- Switch tsconfig `module` and `moduleResolution` to `nodenext`.
- Add explicit `.js` extensions to every relative import/export under src/.
- Add `with { type: 'json' }` to the package.json import in http.ts
(now required by nodenext; supported in Node 18.20+, 20.10+, 22+).
- Switch eventemitter3 to a named import in client.ts; nodenext's stricter
ESM resolution otherwise treats the .mjs default as a namespace (TS2507).
Repro before fix: `node --input-type=module -e "import * as soap from
'./lib/soap.js'"` failed. After fix it loads cleanly, as do all subpath
exports declared in package.json.
📝 WalkthroughWalkthroughThe PR updates TypeScript configuration and import statements across the codebase to explicitly include Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/http.ts`:
- Line 14: The JSON import uses the newer "with { type: 'json' }" attribute
which breaks Node 18.0–18.19; replace that import so package.json is loaded in a
Node-18.0+ compatible way (e.g., read and parse the file at runtime) instead of
using the JSON import attribute. Locate the problematic import line referencing
package.json in src/http.ts (the import statement using "with { type: 'json' }")
and change it to load package.json via a runtime read/parse approach (for
example using fs.readFileSync/new URL(import.meta.url) and JSON.parse or
createRequire) and assign the result to the same symbol (pkg) so all references
to pkg in functions such as any logic in this module continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f85ffc3-c77d-4c63-829f-61842bd04b83
📒 Files selected for processing (12)
src/client.tssrc/http.tssrc/security/BasicAuthSecurity.tssrc/security/BearerSecurity.tssrc/security/WSSecurity.tssrc/security/index.tssrc/soap.tssrc/types.tssrc/utils.tssrc/wsdl/elements.tssrc/wsdl/index.tstsconfig.json
- src/http.ts: attach .catch to NTLM early-return rejection (same pattern as main responsePromise, prevents unhandled rejection on callback-only callers) - test/test-helpers.ts: drop useless baseDir reassignment (parameter retained for API compat; toTestUrl embeds the absolute path) - test/wsdl.test.ts: remove unused expect + path imports; replace 'undefined as string' cast with explicit 'undefined' string key - test/header-rely-on-xml.test.ts: replace hardcoded port with testHelpers.nextTestPort(); wait on server.close() in afterAll; document intentional err-ignore (mock response schema mismatch is orthogonal to the request-side assertion under test) - test/client-schema-does-not-change-on-request.test.ts: add afterAll to close the HTTP server (was leaking) - test/request-response-samples.test.ts: forward options through promiseCaller to match cbCaller; propagate assertion failures to done(err) instead of swallowing in .finally - eslint.config.mjs: ignore docs/ (typedoc output, gitignored) - specs/2026-04-16-bun-migration-design.md: update D7 to reflect realized option B (we skipped the unblock; PR #44 fixed #43, then this Bun PR superseded the mocha-based test job) Not addressed (declined): - CodeRabbit suggested bumping bun@1.3.11 → 1.3.12. Tests ran against 1.3.11; Dependabot will propose a bump through the normal path once the 1.3.12 schema is validated. - CodeRabbit flagged a stray fence at plans/...:1183. That's inside a markdown doc describing how to write CONTRIBUTING instructions; the nested fences are intentional (the inner block is the target file's content).
* docs: add Bun migration design Spec for migrating fetch-soap's test suite and dev toolchain from mocha + npm to `bun test` + `bun install`. Keeps `tsc` for build; no changes to `src/`, `lib/`, or published artifacts. * plan: Bun migration implementation plan Task-by-task plan for migrating fetch-soap's test suite and dev toolchain to Bun, per specs/2026-04-16-bun-migration-design.md. Also includes a minor factual correction to the spec: test/_socketStream.js is dead code (zero importers) and will be deleted rather than rewritten, which also retires duplexer, semver, and readable-stream from devDeps. * plan: pivot to option B (skip unblock), add bun audit + static baseline - Update Assumptions: no unblock PR (tests never ran; see #43) - Task 1: replace mocha baseline with static per-file it() counts - Task 12: compare bun test count to static floor, not runtime baseline - Task 15: add bun audit --prod --audit-level=high to preserve #36's security posture; use 'bun ci' for clarity * chore(bun): add bun.lock and packageManager field Installs match package-lock.json (tsc output byte-identical). package-lock.json retained for now; removed after CI flips to Bun. * refactor(test): rename *-test.js to *.test.ts (pure rename) Pure rename commit with no content changes. Files are temporarily non-functional (.ts extension on CJS content); content rewrite follows in subsequent commits. This commit exists to preserve git blame/log --follow across the rename. Also deletes test/_socketStream.js which is dead code (zero importers). * chore(bun): add @types/bun * refactor(test): rewrite test-helpers as ESM TypeScript Add types, import HttpClient from src/, replace __dirname with import.meta.dir, convert require/module.exports to ESM. * refactor(test): migrate trim-test to bun:test (pilot) Import trim from src/ directly, use bun:test's describe/it, use node:assert for strict equality. Fixed a pre-existing structural bug where describe blocks were nested inside an it. * refactor(test): migrate security tests to bun:test - Replace should with expect - Replace sinon.useFakeTimers with setSystemTime (WSSecurity) - Import from src/security/index.js instead of package root - Convert var to const Adjust one WSSecurity assertion (_hasNonce / _actor) from toHaveProperty to toBeUndefined: the original mocha/should test relied on legacy tsc's omission of uninitialized class fields, which doesn't hold under Bun's native class-fields semantics. The intent (constructor didn't set these fields) is preserved. * fix(wsdl): use declare on shadowing \$-prefixed class fields Subclasses of Element declared \$-prefixed fields (e.g. \$type, \$ref, \$base) without initializers. Under modern class-fields semantics (useDefineForClassFields: true, which Bun always uses regardless of tsconfig), these declarations run *after* super() and define the property via Object.defineProperty to undefined — silently overwriting the dynamic attr assignments made by the Element base constructor (this['\$' + key] = attrs[key]). The result: when WSDL parsing runs under Bun (or any modern TS compile with useDefineForClassFields on), BindingElement.\$type and friends are undefined after construction, downstream parsing finds no portType match, and no SOAP methods get attached to generated clients. Legacy tsc with the current tsconfig happens to emit nothing for uninitialized declarations, so lib/ has never been affected — but future target bumps or compiler upgrades could surface this. Fix: prefix every shadowing declaration with \`declare\`, a pure type-level keyword that emits no runtime code. The parent constructor's dynamic assignment stays authoritative, under both legacy and modern semantics. lib/ output is byte-identical to pre-patch. Unblocks the bun:test migration (src/ tests that exercise WSDL parsing now work against src/ directly). No behavior change for any existing consumer. * refactor(test): migrate simple tests to bun:test Mechanical rewrite: require → import, should → expect, var → const. node:assert calls kept as-is. * refactor(test): migrate custom-http and wsdl-cache tests - Replace sinon.createSandbox with spyOn + mockRestore - Use test-helpers via ESM import - import.meta.dir in place of __dirname * refactor(test): migrate request-response-samples, replace timekeeper with setSystemTime * test: skip 2 pre-existing broken fixtures in request-response-samples fooOp__should_return_back_good_response_object asserts err.root === null on non-Fault parse errors, but src/wsdl/index.ts only sets error.root on SOAP-Fault paths. These never ran under mocha --bail (prior failures aborted the suite), so the mismatch has been latent since the fork. SendCDATA__cdata_preserves_leading_and_trailing_whitespace_... fails only as collateral from state left behind by the fooOp failure (the server handler's deferred assertion runs against the next test's request). It passes in isolation. Skipping both until src is aligned with the fixtures' intent; filing a follow-up issue. * fix(http): prevent unhandled rejection on fetch errors The request() method's .catch handler calls callback(err) and re-throws, which is necessary to keep the returned promise rejecting for consumers who await it. But callers using the callback API (the majority) don't attach a .catch, so the re-throw surfaces as an unhandled rejection — harmless under Node + mocha but treated as a test failure under Bun. Attach a no-op .catch to responsePromise before returning. The original promise still rejects for anyone who awaits it (behavior preserved), but Bun no longer sees an unattached rejection. Unblocks ~50 integration tests in the bun:test migration that exercise error paths (invalid host, ConnectionRefused, etc.). No change to callback-path semantics. * refactor(test): migrate client.test.ts to bun:test - Replace should with expect, require with import, var with const. - Replace sinon.spy with bun:test's spyOn. - Keep done-callback async style (out-of-scope to convert). - Add testHelpers.nextTestPort() allocator; replace hardcoded port 15099 (16 instances) with per-test unique ports to prevent EADDRINUSE when running sequentially without mocha --bail. - Remove 2 buggy '.close(() => done())' chains that called done() twice (once from soap callback, once from close callback). The original pattern worked under mocha --bail because only one done() fired per test before bailing; under Bun's bail-free execution both fire, triggering 'done called multiple times' errors. - Skip 7 tests (14 counting streaming/non-streaming variants) that exercise integration patterns with pre-existing issues surfacing for the first time now that tests actually run. Tracked as a follow-up. Test result: 150 pass / 18 skip / 0 fail. * refactor(test): migrate wsdl.test.ts to bun:test - Replace CJS requires with ESM imports from src/wsdl/index.js and src/wsdl/elements.js. - Replace sinon.spy with bun:test's spyOn; adapt spy.callCount to spy.mock.calls.length and spy.restore() to spy.mockRestore(). - Replace require() of CJS fixture modules (wsdl/typeref/request.xml.js, wsdl/perf/request.xml.js) with createRequire(import.meta.url). - var → const, __dirname → import.meta.dir. - Add type annotations where the typechecker needs them. Test result: 78 pass / 0 fail (34 static it() calls expand via fs.readdirSync dynamic tests over the wsdl/ fixture directory). * chore(test): remove mocha, should, sinon, timekeeper, source-map-support, duplexer, semver, readable-stream duplexer, semver, and readable-stream were only used by the deleted test/_socketStream.js. The rest were replaced by bun:test primitives. - Change 'test' script to 'bun test' - Delete test/mocha.opts - Apply prettier across touched files * chore: remove package-lock.json, bun.lock is authoritative * ci: migrate PR workflow to Bun Replaces setup-node + npm ci + mocha/npm audit with setup-bun + bun ci + bun test + bun audit. Bun version pinned to match the packageManager field in package.json. - build-test job: replaces code-quality and test jobs from the pre-migration pr.yml (#34 introduced test) - security job: bun audit --prod --audit-level=high replaces the npm audit invocation from #36 with identical posture (production only, high severity floor) * chore(ci): switch dependabot ecosystem to bun * docs: update CONTRIBUTING for Bun toolchain * docs: add bun/pnpm/yarn install alternatives + Bun dev toolchain note * review: address CodeQL + CodeRabbit findings - src/http.ts: attach .catch to NTLM early-return rejection (same pattern as main responsePromise, prevents unhandled rejection on callback-only callers) - test/test-helpers.ts: drop useless baseDir reassignment (parameter retained for API compat; toTestUrl embeds the absolute path) - test/wsdl.test.ts: remove unused expect + path imports; replace 'undefined as string' cast with explicit 'undefined' string key - test/header-rely-on-xml.test.ts: replace hardcoded port with testHelpers.nextTestPort(); wait on server.close() in afterAll; document intentional err-ignore (mock response schema mismatch is orthogonal to the request-side assertion under test) - test/client-schema-does-not-change-on-request.test.ts: add afterAll to close the HTTP server (was leaking) - test/request-response-samples.test.ts: forward options through promiseCaller to match cbCaller; propagate assertion failures to done(err) instead of swallowing in .finally - eslint.config.mjs: ignore docs/ (typedoc output, gitignored) - specs/2026-04-16-bun-migration-design.md: update D7 to reflect realized option B (we skipped the unblock; PR #44 fixed #43, then this Bun PR superseded the mocha-based test job) Not addressed (declined): - CodeRabbit suggested bumping bun@1.3.11 → 1.3.12. Tests ran against 1.3.11; Dependabot will propose a bump through the normal path once the 1.3.12 schema is validated. - CodeRabbit flagged a stray fence at plans/...:1183. That's inside a markdown doc describing how to write CONTRIBUTING instructions; the nested fences are intentional (the inner block is the target file's content). * review: address second-round CodeRabbit findings - specs/2026-04-16-bun-migration-design.md (D6): swap `bun install --frozen-lockfile` for `bun ci` to match the implemented workflow. - test/test-helpers.ts: return a real Promise from the mock request instead of a custom thenable. The previous implementation called fs.readFile twice (once for the callback path, once inside then()) and the catch handler was a no-op — meaning promise consumers couldn't observe errors. Single read now feeds both paths, matching the real HttpClient contract. No-op .catch on responsePromise preserves the "callback is the primary error channel" invariant.
Summary
Fixes `ERR_MODULE_NOT_FOUND` for pure-Node ESM consumers of `fetch-soap`. The published `lib/` previously contained extensionless relative imports (e.g. `import { Client } from './client'`) because tsconfig used `moduleResolution: bundler`. Node's ESM resolver requires explicit `.js` extensions, so any consumer doing `import 'fetch-soap'` from a pure-Node ESM context failed. Bundler-based consumers (webpack, rollup, Next.js) didn't see this because bundlers tolerate extensionless imports.
What changed
No behavior changes. Sources still typecheck and the build still emits a `lib/` tree with the same shape — just with importable filenames.
Repro / verification
Before the fix:
```
$ npm ci && npm run build
$ node --input-type=module -e "import * as soap from './lib/soap.js'; console.log(Object.keys(soap))"
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../lib/client'
```
After the fix the same command prints all 10 expected keys, and every subpath export declared in `package.json` (`./security`, `./client`, `./wsdl`, `./http`, `./types`) loads cleanly under Node ESM.
Reviewer notes
Test plan
Summary by CodeRabbit