Skip to content

refactor(dapi)!: convert dapi-client to ESM, drop webpack/karma#3681

Closed
PastaPastaPasta wants to merge 3 commits into
v3.1-devfrom
claude/esm-3-dapi-client
Closed

refactor(dapi)!: convert dapi-client to ESM, drop webpack/karma#3681
PastaPastaPasta wants to merge 3 commits into
v3.1-devfrom
claude/esm-3-dapi-client

Conversation

@PastaPastaPasta
Copy link
Copy Markdown
Member

Summary

Converts @dashevo/dapi-client to pure ESM. Removes the webpack/karma browser-bundle infrastructure. Consumers must now use a modern bundler (Vite/esbuild/webpack 5) that handles ESM natively.

This is PR 3 of 5 in the stacked series. Depends on #3679 and #3680.

Warning

This PR temporarily breaks wallet-lib and js-dash-sdk tests. Both packages currently require() dapi-client, and CJS cannot require an ESM module. PRs 4 and 5 in this stack convert those packages to ESM and unbreak them. The three PRs (3, 4, 5) should land together.

What changes

dapi-client

  • package.json: "type": "module", "exports": { ".": "./lib/index.js", "./lib/*": "./lib/*" }, drops main. The wildcard preserves the deep-import paths that wallet-lib and js-dash-sdk use.
  • All ~115 source files and ~80 test files converted from CJS require/module.exports to ESM import/export. All relative imports include the .js extension (ESM requirement).
  • events npm package moved from devDependencies → dependencies. Used by DAPIClient, ReconnectableStream, BlockHeadersProvider for EventEmitter. The npm events package works in both Node and browser bundlers.
  • Deletes webpack.config.js, karma.conf.js, lib/test/karma/. Removes @babel/core, babel-loader, all karma-*, all browser-polyfill devDeps (assert-browserify, buffer, crypto-browserify, os-browserify, path-browserify, process, stream-browserify, string_decoder, url, util, webpack, webpack-cli).
  • .mocharc.yml uses import: for the ESM bootstrap.

Bootstrap

  • lib/test/bootstrap.js converted to ESM with a defensive loadDpp.default ?? loadDpp unwrap (wasm-dpp is CJS; NodeNext interop sometimes wraps the default in a namespace object).

Test plan

  • yarn workspace @dashevo/dapi-client run test:unit315 passing
  • yarn workspace @dashevo/wallet-lib run test:unit — 364/377, 13 failing (expected; PR 4 fixes)
  • yarn workspace dash run test:unit — TS compile fails on Cannot find module '@dashevo/dapi-client' (expected; PR 5 fixes)

Breaking changes

  • Package is ESM-only. Any require('@dashevo/dapi-client') breaks.
  • Deep imports require .js extensions: import X from '@dashevo/dapi-client/lib/transport/ReconnectableStream.js'.
  • No prebuilt browser bundle. Consumers must use a bundler.
  • Drops devDeps that downstream consumers might have transitively relied on for browser polyfills. Consumers needing those must add them to their own devDeps.

Stack

@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: e0172a6e-d52b-4513-9391-26f1a0257481

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-3-dapi-client

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: "ccc4eaf80c19bbdae4b8e6fd11065f9137cf30284e649ef3b88972d5337deb78"
)

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

I reviewed the dapi-client ESM conversion at befadd8e30098c41d5f25d3bc61a48fac6cffdec. The source conversion itself looks mechanically consistent, but I found two release-surface issues worth fixing before publishing this as the new supported package shape: the package docs still tell users to require() an ESM-only entrypoint, and the removed Karma/Webpack browser harness is not replaced by even a minimal modern-bundler smoke test.

Reviewed commit: befadd8

🟡 2 suggestion(s)

1. suggestion: Update published examples before making the package ESM-only (packages/js-dapi-client/package.json:5)

This line makes @dashevo/dapi-client ESM-only, but the published package docs still show CommonJS usage such as const DAPIClient = require('@dashevo/dapi-client'); in packages/js-dapi-client/README.md and packages/js-dapi-client/docs/getting-started/quickstart.md. Consumers copying those examples after this release will immediately hit ERR_REQUIRE_ESM. Please update the package README/docs in the same PR (for example, import DAPIClient from '@dashevo/dapi-client';) so the shipped documentation matches the new entrypoint format.

2. suggestion: Add a replacement browser/bundler smoke test for the new support model (packages/js-dapi-client/package.json:63)

The PR removes test:browsers, build:web, Karma, Webpack, and the prebuilt browser bundle, so CI no longer exercises any browser-oriented path. The PR description says consumers should now use modern bundlers such as Vite/esbuild/Webpack 5, but there is no smoke test proving that the ESM package can be resolved and executed through that path. Given this package still pulls in events, google-protobuf, generated gRPC modules, and wasm loading, a tiny bundler/browser smoke test would catch regressions that Node-only Mocha coverage cannot.

Note: inline posting hit GitHub HTTP 422, so I’m posting the verified findings in the top-level review body.

@PastaPastaPasta PastaPastaPasta force-pushed the claude/esm-3-dapi-client branch from befadd8 to c830cc9 Compare May 19, 2026 19:45
@PastaPastaPasta PastaPastaPasta changed the title refactor(dapi-client)!: convert to ESM, drop webpack/karma refactor(dapi)!: convert dapi-client to ESM, drop webpack/karma 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

Verified all reviewer findings against the worktree at c830cc9. One blocking issue is real: .mocharc.yml still preloads the now-ESM bootstrap with require:, which will break the package's own unit-test command under Node 18.18 (the declared engine floor) since require() of an ESM module throws ERR_REQUIRE_ESM. README/docs still document CommonJS usage, the deleted browser bundle is still referenced by examples/web/web.usage.html, the wasm-dpp interop in createGrpcTransportError.js contradicts the defensive unwrap pattern bootstrap.js was written with, and the new bundler-only support model has no smoke-test coverage. No CodeRabbit findings were provided.

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)

5 additional finding(s)

blocking: Mocha still loads the ESM bootstrap with `require:`, breaking the package test command

packages/js-dapi-client/.mocharc.yml (line 1)

packages/js-dapi-client/.mocharc.yml still preloads ./lib/test/bootstrap.js with require:, but the package is now "type": "module" and lib/test/bootstrap.js uses ESM (import statements and export const mochaHooks). With the declared engine floor of node: ">=18.18", Node 18 does not support require(esm) and will throw ERR_REQUIRE_ESM before any test runs. The PR description states the bootstrap was switched to import:, but the change did not actually land on this head. Replace require: with import: so the existing ESM root-hooks file loads correctly, otherwise yarn workspace @dashevo/dapi-client run test:unit aborts before executing the suite.

suggestion: Published docs still advertise removed CommonJS / CDN entrypoints

packages/js-dapi-client/README.md (line 35)

This package becomes ESM-only on this PR ("type": "module", no main, no browser bundle), but every usage snippet in packages/js-dapi-client/README.md (lines 35, 48, 69, 88) still uses const DAPIClient = require('@dashevo/dapi-client');. The same stale guidance lives in packages/js-dapi-client/docs/getting-started/quickstart.md:26 and packages/js-dapi-client/docs/usage/application/DAPIClient.md:19, and some docs still advertise the old unpkg/CDN standalone path even though this PR removes the browser bundle. Because README.md and docs/ are both shipped in the published package (files: ["docs", "lib"]), consumers copying the snippets after this release will immediately hit ERR_REQUIRE_ESM or look for an artifact that no longer exists. Update the four README examples and the two docs files in the same PR to ESM (import DAPIClient from '@dashevo/dapi-client';) so the shipped documentation matches the new entrypoint shape.

suggestion: No smoke-test coverage for the new bundler-only support model

packages/js-dapi-client/package.json (line 61)

This PR removes build:web, test:browsers, karma, webpack, and the prebuilt browser bundle, while the PR description promises that modern bundlers (Vite/esbuild/Webpack 5) handle the package natively. However, the only test scripts left in packages/js-dapi-client/package.json run Node-based Mocha (test:unit, test:integration, test:coverage); nothing in CI exercises a bundler against lib/index.js. Given the package pulls in events, google-protobuf, generated gRPC modules, and wasm-dpp loading, a tiny Vite/esbuild/Webpack 5 smoke build plus a new DAPIClient() import would catch regressions Node-only Mocha cannot — e.g. future code reaching for Node-only APIs (Buffer, node:fs) or a stale deep-import path inside the ./lib/* wildcard export.

suggestion: wasm-dpp interop here contradicts the defensive unwrap in lib/test/bootstrap.js

packages/js-dapi-client/lib/transport/GrpcTransport/createGrpcTransportError.js (line 6)

lib/test/bootstrap.js:13-17 was deliberately written to handle two shapes for the @dashevo/wasm-dpp default import:

// wasm-dpp is CJS; under NodeNext the default may resolve to the namespace
// object instead of the callable. Unwrap defensively.
const load = loadDpp.default ?? loadDpp;
return load();

But createGrpcTransportError.js:17 consumes the same module assuming only the namespace shape:

const { deserializeConsensusError, default: loadWasmDpp } = wasmDpp;

If the bootstrap's concern is real — i.e. interop sometimes hands back the callable directly rather than the namespace wrapper — then wasmDpp.default is undefined and loadWasmDpp() on line 51 will throw loadWasmDpp is not a function the first time the error path through this module runs in production. Tests currently pass because today's Node + bootstrap path lands on the namespace shape, but this inconsistency is exactly the failure mode bootstrap was written to dodge. Either drop the defensive unwrap in bootstrap (if it's truly unnecessary), or apply the same default ?? wasmDpp style here and at the other call sites that read Identifier / deserializeConsensusError directly off the default import (lib/methods/platform/getIdentitiesContractKeys/GetIdentitiesContractKeysResponse.js, test/**/*.spec.js) so all callers agree on wasm-dpp's export shape.

nitpick: Web example still references the deleted prebuilt bundle

packages/js-dapi-client/examples/web/web.usage.html (line 7)

examples/web/web.usage.html loads <script src="../../dist/dapi-client.min.js"></script>, but this PR removes build:web, webpack.config.js, and the dist/ folder from the published files list — there is no longer any way to produce dist/dapi-client.min.js. The example is broken on first load and contradicts the new bundler-required guidance. Either delete this example or rewrite it against a representative modern-bundler setup (Vite/esbuild) consistent with the new support model.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

…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).
@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-3-dapi-client 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