Skip to content

fix(esm): emit Node-resolvable ESM by switching to nodenext#44

Merged
evans-sam merged 1 commit into
masterfrom
claude/gallant-swanson-aff4d0
Apr 16, 2026
Merged

fix(esm): emit Node-resolvable ESM by switching to nodenext#44
evans-sam merged 1 commit into
masterfrom
claude/gallant-swanson-aff4d0

Conversation

@evans-sam
Copy link
Copy Markdown
Owner

@evans-sam evans-sam commented Apr 16, 2026

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

  • `tsconfig.json`: `module` and `moduleResolution` switched to `nodenext`.
  • All relative imports/exports under `src/` now carry explicit `.js` extensions.
  • `src/http.ts`: package.json import gained `with { type: 'json' }` — required by nodenext for JSON imports. Supported in Node 18.20+, 20.10+, 22+.
  • `src/client.ts`: `eventemitter3` switched to a named import. Nodenext's stricter ESM resolution otherwise treats the `.mjs` default as a module namespace (TS2507).

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

  • Engines field is currently `>=18.0.0`. Strictly speaking, the new JSON import attribute (`with { type: 'json' }`) is only supported on Node 18.20+ for the 18.x line. Worth bumping in a follow-up if precision matters; not done here.
  • `test/` was intentionally not touched. `npm test` currently fails because the test files use CJS `require()` in a `"type": "module"` package — preexisting and presumably handled by the in-flight Bun migration (`plans/2026-04-16-bun-migration.md`).

Test plan

  • CI build passes
  • `npm run build && node --input-type=module -e "import * as soap from './lib/soap.js'; console.log(Object.keys(soap))"` prints keys (no `ERR_MODULE_NOT_FOUND`)
  • Each subpath export under `package.json#exports` is importable from a Node ESM consumer

Summary by CodeRabbit

  • Refactor
    • Updated module imports to use explicit file extensions and ESM-compatible paths across the codebase.
    • Adjusted TypeScript compiler configuration for improved module resolution standards.

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 16, 2026

📝 Walkthrough

Walkthrough

The PR updates TypeScript configuration and import statements across the codebase to explicitly include .js file extensions and use ESM-style module resolution. Configuration changes from ESNext/bundler to nodenext for both module and moduleResolution settings.

Changes

Cohort / File(s) Summary
TypeScript Configuration
tsconfig.json
Module system changed from ESNext to nodenext and moduleResolution from bundler to nodenext to align with ESM module resolution standards.
Core Client & SOAP Module
src/client.ts, src/soap.ts
Import specifiers updated with .js extensions (./http.js, ./types.js, ./utils.js, ./wsdl/index.js, etc.); named import for EventEmitter from eventemitter3; re-exports adjusted to match new specifiers.
HTTP & Type Utilities
src/http.ts, src/types.ts, src/utils.ts
Import paths updated to use explicit .js extensions; JSON module assertion added to package.json import in http.ts.
Security Module
src/security/BasicAuthSecurity.ts, src/security/BearerSecurity.ts, src/security/WSSecurity.ts, src/security/index.ts
All import specifiers updated with .js extensions; re-export paths in index.ts adjusted to reference .js files.
WSDL Module
src/wsdl/elements.ts, src/wsdl/index.ts
Import specifiers updated with .js extensions for all internal and sibling module references.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Poem

🐰 Extensions sprouting in each import line,
From .js whispers, modules align,
nodenext paths through forests we roam,
ESM standards bring clarity home! 📦✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: converting to Node-resolvable ESM via the nodenext TypeScript configuration, with explicit .js extensions and import syntax updates throughout the codebase.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/gallant-swanson-aff4d0

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6f4460a and 2231649.

📒 Files selected for processing (12)
  • src/client.ts
  • src/http.ts
  • src/security/BasicAuthSecurity.ts
  • src/security/BearerSecurity.ts
  • src/security/WSSecurity.ts
  • src/security/index.ts
  • src/soap.ts
  • src/types.ts
  • src/utils.ts
  • src/wsdl/elements.ts
  • src/wsdl/index.ts
  • tsconfig.json

Comment thread src/http.ts
@evans-sam evans-sam merged commit 79b58e2 into master Apr 16, 2026
5 of 6 checks passed
evans-sam added a commit that referenced this pull request Apr 17, 2026
- 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).
evans-sam added a commit that referenced this pull request Apr 17, 2026
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant