Skip to content

refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib#3683

Closed
PastaPastaPasta wants to merge 5 commits into
v3.1-devfrom
claude/esm-5-sdk-and-test-suite
Closed

refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib#3683
PastaPastaPasta wants to merge 5 commits into
v3.1-devfrom
claude/esm-5-sdk-and-test-suite

Conversation

@PastaPastaPasta
Copy link
Copy Markdown
Member

Summary

Final PR in the ESM migration stack. Converts dash (@dashevo/js-dash-sdk) and @dashevo/platform-test-suite to ESM. Restores monorepo-wide green tests after PR 3.

This is PR 5 of 5. Depends on #3679, #3680, #3681, #3682.

What changes

js-dash-sdk

  • package.json: "type": "module", exports map, engines.node >= 18.18. Drops unpkg/browser, test:browsers*, prepublishOnly. Removes devDeps: all karma-*, webpack, browser polyfills, ts-mock-imports (replaced).
  • tsconfig.build.json and tsconfig.json: module: nodenext, moduleResolution: nodenext, target: es2020.
  • All 54 .ts source files get .js extensions on relative imports (TS NodeNext requirement: TS compiles .ts to .js, but the import string must already say .js).
  • Refactored src/SDK/Client/Platform/methods/names/register.ts to accept an injectable randomBytes parameter instead of relying on ts-mock-imports. ESM module exports are frozen; ts-mock-imports-style export mutation no longer works.
  • Replaced winston with console-backed logger.
  • CJS-named-import fixes for @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common, and the system-id imports from contract packages (@dashevo/dpns-contract, @dashevo/dashpay-contract, etc.).
  • Added parallel import type { X as XType }; type X = XType; aliases in 22 .ts files where destructured value names are also used in type positions. Works around TS2440 namespace collision under module: nodenext.
  • Defensive (loadDpp.default ?? loadDpp)() unwrap in 4 sites where wasm-dpp's CJS default is called directly (Platform.initializeDppModule, createIdentityFixtureInAccount, two contract specs).
  • Deletes webpack.config.js, webpack.base.config.js, karma/.

platform-test-suite

  • package.json: "type": "module", drops test:browsers, browser polyfills, karma-*.
  • All ~35 .js files converted to ESM with .js extensions on relative imports.
  • lib/test/bootstrap.js rewritten as ESM with fileURLToPath-based __dirname.
  • .mocharc.yml uses import: for ESM root hooks.
  • Deletes karma.conf.js, lib/test/karma/.

Test plan

  • yarn workspace @dashevo/dapi-client run test:unit315 passing
  • yarn workspace @dashevo/wallet-lib run test:unit377 passing
  • yarn workspace dash run test:unit60 passing ✅ (3 pending pre-existing)
  • yarn workspace @dashevo/platform-test-suite run lint — ESM bootstrap loads
  • platform-test-suite e2e tests require live testnet credentials in .env (pre-existing constraint, unrelated to this conversion)

Breaking changes

  • Both packages ESM-only. require('dash') and require('@dashevo/platform-test-suite') no longer work.
  • Deep imports require .js extensions.
  • js-dash-sdk's Platform.names.register(...) gained an optional trailing randomBytes parameter. Existing callers unaffected; subclasses or replacements need to know.
  • Drops winston, bs58, node-inspect-extracted from deps where unused.

Monorepo state after this stack lands

  • @dashevo/dapi-client — ESM, Uint8Array API, browser-bundler-friendly (Vite/esbuild/webpack 5)
  • @dashevo/wallet-lib — ESM
  • dash — ESM TypeScript
  • @dashevo/platform-test-suite — ESM
  • @dashevo/dashmate — was already ESM, works as-is
  • @dashevo/dapi-grpc — patched in PR 1; rest of package unchanged

Stack

@github-actions github-actions Bot added this to the v3.1.0 milestone May 19, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 58300c05-01e0-4fc3-bf6e-d0e0ae030109

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/esm-5-sdk-and-test-suite

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

✅ DashSDKFFI.xcframework built for this PR.

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "cb65af56c972a09767f118abb19fc931d30ec8a8e46494a5c05a0817626e9e6b"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

The ESM conversion in js-dash-sdk is not self-consistent at this SHA. The new package boundary breaks existing in-repo deep imports, and the package is now published as ESM-only while at least one unchanged CommonJS runtime consumer still loads it with require(), so the monorepo contains executable paths that will fail after this change.

Reviewed commit: 68e85b41

🔴 2 blocking

GitHub's PR diff API returned PullRequest.diff too_large for this PR (>300 files), so these verified findings are posted in the top-level review body instead of inline comments.

1. blocking: The export map rewrites existing `dash/build/...` imports to non-existent paths

packages/js-dash-sdk/package.json (lines 7-9)

The new subpath export only defines "./*": "./build/*". For any existing import that already includes build/, Node substitutes the wildcard after ./, so dash/build/SDK/Client/Platform/signStateTransition.js resolves to ./build/build/SDK/Client/Platform/signStateTransition.js. This PR leaves live callers on that import form in packages/platform-test-suite/test/functional/platform/Document.spec.js:4, packages/platform-test-suite/test/functional/platform/Identity.spec.js:3, packages/platform-test-suite/test/e2e/withdrawals.spec.js:4, and packages/bench-suite/benchmarks/fees/dataContractCreate.js:1, so those paths stop resolving as soon as the export map is enforced.

Suggested replacement:

  "exports": {
    ".": "./build/index.js",
    "./build/*": "./build/*",
    "./*": "./build/*"
  }
2. blocking: Switching `dash` to ESM-only breaks unchanged CommonJS consumers in the workspace

packages/js-dash-sdk/package.json (lines 5-9)

Adding "type": "module" makes build/index.js load as ESM, so require('dash') now throws instead of returning the SDK. This PR does not migrate all internal runtime consumers: packages/bench-suite/lib/client/createClientWithFundedWallet.js:1 still does const Dash = require('dash');, and packages/bench-suite/bin/bench.js is a CommonJS entrypoint that reaches that file. That leaves the benchmark path unusable at this SHA unless dash keeps a CommonJS entrypoint or all remaining CommonJS consumers are converted in the same change.


Note: reviewer session(s) with no parseable output: claude-general. The verifier checked the parseable agent findings against the checked-out SHA before this was posted.

@PastaPastaPasta PastaPastaPasta force-pushed the claude/esm-5-sdk-and-test-suite branch from 68e85b4 to dc7fb60 Compare May 19, 2026 19:46
@PastaPastaPasta PastaPastaPasta changed the title refactor(js-dash-sdk,platform-test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib May 19, 2026
@PastaPastaPasta PastaPastaPasta marked this pull request as draft May 19, 2026 19:56
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

I rechecked the incremental push at dc7fb608 against the current worktree. The latest push has the same tree as the prior reviewed commit 68e85b41, so there are no new delta-specific defects, but both prior blocking findings still reproduce at the current head and are carried forward below.

Reviewed commit: dc7fb608 (dc7fb6086f650b5d85a435b206d700868eeaaf30)

🔴 2 blocking

GitHub's PR diff API returned PullRequest.diff too_large for this PR (>300 files), so these verified findings are posted in the top-level review body instead of inline comments.

Prior finding reconciliation

  • STILL VALID: prior #1 export map rewrites dash/build/... imports to non-existent ./build/build/... paths. Verified the package export map and live callsites still exist at dc7fb608.
  • STILL VALID: prior #2 ESM-only dash still breaks unchanged CommonJS workspace consumers. Verified bench-suite remains CommonJS and still calls require('dash'); wasm-drive-verify also still has a require('dash') integration test.

New findings in latest delta

No new delta-specific findings. The latest push did not change the checked tree relative to 68e85b41; the review below carries forward the unresolved prior blockers.

1. blocking: Export map rewrites `dash/build/...` deep imports to non-existent `./build/build/...` paths

packages/js-dash-sdk/package.json (lines 7-10)

The exports block defines only ".": "./build/index.js" and the wildcard "./*": "./build/*". Under Node's subpath export pattern matching, a consumer importing dash/build/<sub> captures build/<sub> for * and resolves to ./build/build/<sub>, which does not exist on disk. The following imports still exist at this head and will fail to resolve:

  • packages/platform-test-suite/test/functional/platform/Identity.spec.js:3import { createFakeInstantLock } from 'dash/build/utils/createFakeIntantLock.js';
  • packages/platform-test-suite/test/functional/platform/Document.spec.js:4import { signStateTransition } from 'dash/build/SDK/Client/Platform/signStateTransition.js';
  • packages/platform-test-suite/test/e2e/withdrawals.spec.js:4import { STATUSES as WITHDRAWAL_STATUSES } from 'dash/build/SDK/Client/Platform/methods/identities/creditWithdrawal.js';
  • packages/bench-suite/benchmarks/fees/dataContractCreate.js:1require('dash/build/SDK/Client/Platform/signStateTransition').

Since platform-test-suite is itself being migrated in this PR, this is a self-inconsistency: the new ESM tests cannot load through their own package boundary. Either drop the build/ prefix at the callsites or add an explicit "./build/*": "./build/*" mapping ahead of the catch-all so existing deep imports continue to resolve.

Suggested replacement:

  "exports": {
    ".": "./build/index.js",
    "./build/*": "./build/*",
    "./*": "./build/*"
  },
2. blocking: ESM-only `dash` breaks unconverted CommonJS workspace consumers

packages/js-dash-sdk/package.json (lines 5-10)

Setting "type": "module" plus an exports map with no require condition makes require('dash') throw ERR_REQUIRE_ESM at runtime. The PR does not migrate every internal consumer:

  • packages/bench-suite/package.json has no "type": "module" (verified — the package is still CommonJS), yet packages/bench-suite/lib/client/createClientWithFundedWallet.js:1 does const Dash = require('dash'); and is reached from the CommonJS packages/bench-suite/bin/bench.js entrypoint. The bench script is consequently unrunnable.
  • packages/bench-suite/benchmarks/fees/dataContractCreate.js:1 additionally uses require('dash/build/...'), compounding the breakage with the export-map issue above.
  • packages/wasm-drive-verify/tests/integration/fetch_testnet_proofs.js:12 uses require('dash'); this PR removes the last CJS-callable entrypoint, so the file can no longer load dash without conversion.

Either convert the remaining CJS callers (bench-suite and the wasm-drive-verify integration test) in the same change, or expose a require-callable entrypoint via dual exports (e.g. ".": { "import": "./build/index.js", "require": "./build/index.cjs" }). The PR description only claims yarn workspace dash run test:unit passes; it does not address bench-suite, so the workspace is left in a broken state.

…ton/fetch/promisify shims

Non-breaking cleanup pass; package stays CJS, no public API changes, no consumer changes required.

dapi-client: replace winston with a minimal console-backed logger that preserves the same API (.error/.warn/.info/.verbose/.debug/.silly/.getForId). Drop node-fetch and the lib/test/bootstrap setimmediate shim — Node 18+ has both globally. Drop the https.Agent self-signed-TLS branch from requestJsonRpc (was Node-only; consumers wanting this must configure NODE_TLS_REJECT_UNAUTHORIZED at the app layer). Inline lodash/sample in ListDAPIAddressProvider. Add engines.node >=18.18. Remove dependencies: winston, node-fetch, lodash, bs58 (unused), node-inspect-extracted (unused). Remove devDeps: setimmediate.

dapi-grpc: inline the promisify shim in core/v0/web/CorePromiseClient.js and platform/v0/web/PlatformPromiseClient.js so the browser bundle no longer requires Node's util module. Both files document the shim so a future codegen regen does not silently reintroduce require('util').
… API

Adds lib/utils/bytes.js helper (hexToBytes/bytesToHex/base64ToBytes/bytesToBase64/concatBytes/bytesEqual) and converts all Buffer.* call sites in dapi-client lib/ to Uint8Array, with corresponding test updates. Package stays CJS.

Production exceptions where Buffer is retained: BlockHeadersReader passes Buffer to dashcore-lib's BlockHeader (its BufferReader needs .readInt32LE), and GetIdentitiesContractKeysResponse passes Buffer to wasm-dpp's Identifier.from (it explicitly requires Node Buffer).

createGrpcTransportError now handles both raw bytes (grpc-js path) and base64 strings (grpc-web path) for drive-error-data-bin, stack-bin, and dash-serialized-consensus-error-bin metadata fields, restoring the dual-format behavior that Buffer.from(x, 'base64') used to provide implicitly.

Test updates: spec files that construct expected protobuf requests now wrap .toBuffer() with new Uint8Array(...) to match production's normalization (sinon calledOnceWithExactly distinguishes Buffer from plain Uint8Array).

Breaking change for direct consumers: response object byte fields are now Uint8Array. Callers that do response.field.toString('hex') will fail — use bytesToHex(response.field) from lib/utils/bytes instead. Buffer.isBuffer(response.field) now returns false; use response.field instanceof Uint8Array.

Test results: dapi-client 315/315, wallet-lib 377/377, js-dash-sdk 60/60 — downstream consumers continue passing without modification (they exercise dapi-client mostly via mocks).
Converts @dashevo/dapi-client to pure ESM: type: module, exports map with ./lib/* wildcard, all CJS require/module.exports replaced with import/export, .js extensions on relative imports. Engine bumped to >=18.18.

Drops webpack.config.js, karma.conf.js, and lib/test/karma/. Removes the @babel/core, babel-loader, browser-polyfill, karma, and webpack devDeps. Consumers must use a modern bundler (Vite/esbuild/webpack 5) which handles ESM natively.

Moves 'events' npm package from devDependencies to dependencies — used by DAPIClient, ReconnectableStream, and BlockHeadersProvider for EventEmitter, and resolves correctly in both Node and browser bundlers.

BREAKING: CJS consumers (require) no longer work. Downstream consumers wallet-lib (PR 4) and js-dash-sdk (PR 5) are temporarily broken between this PR merging and PRs 4/5 merging — they must land as a sequence. Dashmate is already ESM and continues to work.

Test results: dapi-client 315/315. wallet-lib + js-dash-sdk fail as expected (fixed by PRs 4 + 5).
Converts @dashevo/wallet-lib to pure ESM so it can consume the ESM dapi-client from PR 3. Adds 'type: module' to package.json. All src/, fixtures/, and tests/ files converted from CJS require/module.exports to ESM import/export with .js extensions on relative imports.

Deletes webpack.config.js, karma/, src/test/karma/. Removes browser-polyfill devDeps (buffer, crypto-browserify, stream-browserify, etc.) and webpack/karma. Tests run via Mocha in Node 18+; browser builds are out of scope. Engines: >=18.18.

CJS-named-import fixes: lodash, crypto-js, @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common all use default-import + destructure pattern because Node ESM cannot statically enumerate named exports of CJS modules.

Surfaces three previously-silent CJS bugs that ESM strict mode catches: missing UnknownStrategy export from errors/index.js, missing named exports for coinSelection strategies, and a 'type=' implicit-global in DerivableKeyChain.spec.js.

Defensive (loadDpp.default ?? loadDpp)() unwrap in IdentitySyncWorker.onStart for the same wasm-dpp NodeNext interop reason as PR 3's bootstrap.

Test results: wallet-lib 377/377 passing. dapi-client unchanged (315/315). js-dash-sdk still broken (PR 5 fixes).
…SM dapi-client and wallet-lib

Final PR in the ESM migration stack. Converts the two remaining workspace packages that consumed dapi-client/wallet-lib, restoring monorepo-wide green tests after PR 3.

js-dash-sdk: TypeScript-based package with NodeNext module resolution. Adds 'type: module' + exports map. tsconfig.build.json → module: nodenext, moduleResolution: nodenext, target: es2020. All .ts files get .js extensions on relative imports (TS NodeNext compiles .ts to .js but the import string must already say .js). Drops ts-mock-imports by refactoring register.ts to accept an injectable randomBytes parameter (ts-mock-imports relies on mutable module exports which ESM forbids). Deletes webpack/karma + browser polyfills. Replaces winston with console-backed logger. CJS-named-import conversions for @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common, and the system-id imports from contract packages. Adds parallel 'import type' aliases (via 'import type { X as XType }; type X = XType;') in 22 .ts files where destructured value names are also used as types — works around TS2440 namespace collision under NodeNext.

platform-test-suite: converted ~35 files from CJS to ESM. Test bootstrap converted; .mocharc.yml uses 'import:' for ESM root hooks.

Defensive (loadDpp.default ?? loadDpp)() unwrap in 4 sites (Platform.initializeDppModule, createIdentityFixtureInAccount, contracts/get.spec.ts, contracts/history.spec.ts). createGrpcTransportError-style dual-format byte handling preserved.

Test results: 752 passing across the monorepo (dapi-client 315, wallet-lib 377, js-dash-sdk 60). platform-test-suite is end-to-end and requires live testnet credentials in .env (pre-existing constraint).
@PastaPastaPasta
Copy link
Copy Markdown
Member Author

Closing — combined into #3689 because the three are lockstep (CI fails on any in isolation since CJS can't require ESM, leaving the monorepo in a non-buildable mid-state). The three commits are preserved on that branch for focused review.

@PastaPastaPasta PastaPastaPasta deleted the claude/esm-5-sdk-and-test-suite branch May 19, 2026 23:44
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.

2 participants