refactor(wallet-lib)!: convert to ESM, consume ESM dapi-client#3682
refactor(wallet-lib)!: convert to ESM, consume ESM dapi-client#3682PastaPastaPasta wants to merge 4 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
✅ 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: "019d25a2c76c26e944d29988fb6c33c1e054fa941623908dd8ec320c5bf65bb8"
)Xcode manual integration:
|
f6bb12e to
c96ad91
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR 4/5 of the ESM migration converts wallet-lib's source tree cleanly, but two real release-blocking issues remain: the shipped examples/ are still CJS in a "type": "module" package so they fail at runtime, and the new exports map combined with the ESM-only switch breaks every in-repo CJS consumer that still deep-imports from @dashevo/wallet-lib/src/.... Codex's claim that IdentitySyncWorker will throw because Identity is undefined was verified against Node's ESM↔CJS interop and is incorrect — default-importing a CJS module yields the whole module.exports object, so the destructure works today; a related consistency concern is kept as a suggestion. The new console-based logger is a behavior change worth calling out, and a few cleanup items remain.
Reviewed commit: c96ad912 (c96ad912b300045d5856e4847e3a8b359b5408bd)
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)
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 blocker: switching
wallet-libto ESM with the current exports map still breaks in-repo CommonJS/deep-import consumers. Carried forward below as finding #2. - OUTDATED / still not a bug: the prior wasm-dpp
IdentitySyncWorkerloader concern remains a false positive for today's CJS interop shape; the verified review keeps only a future-compatibility consistency suggestion.
Carried-forward prior findings
1. blocking: ESM switch + restrictive exports map breaks in-repo CJS consumers' deep imports
packages/wallet-lib/package.json (line 5)
Two compounding problems leave existing consumers broken before PR 5 lands:
-
"type": "module"makes wallet-lib ESM-only, but several consumers still load it withrequire(...):packages/platform-test-suite/lib/test/createClientWithFundedWallet.js:3,packages/bench-suite/lib/client/createClientWithFundedWallet.js:5,packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/internal/createAssetLockProof.ts:4, andpackages/js-dash-sdk/src/test/mocks/createAndAttachTransportMocksToClient.ts:14-16. Synchronousrequire()of an ESM module fails withERR_REQUIRE_ESM. -
The new exports map only declares
"./src/*": "./src/*", which requires exact filename matches with extensions and no directory-index fallback. Even after consumers are converted to ESM, paths like@dashevo/wallet-lib/src/utils/fundWallet(used in platform-test-suite/bench-suite) and@dashevo/wallet-lib/src/errors(used in js-dash-sdk) — which currently rely on Node's legacy extension/index resolution — will fail to resolve tofundWallet.jsanderrors/index.jsrespectively.js-dash-sdk/src/SDK/Client/Client.ts:3(@dashevo/wallet-lib/src/transport/DAPIClientTransport/DAPIClientTransport) has the same problem.
Either land the consumer updates atomically with this PR, or expand the exports map to expose deep paths with both extensionless and directory-index forms.
Source: codex
New findings in latest delta
2. blocking: All examples/ files still use CommonJS in an ESM package — they crash on execution
packages/wallet-lib/examples/client-usage.js (line 1)
package.json now sets "type": "module", so Node parses every .js in this package as ESM. Every file under examples/ (client-usage.js, client-usage-single-privateKey.js, offline-wallet.js, offline-wallet-signing-message.js, wallet-fromHDPubKey.js, wallet-plugins.js, stdPlugins/WalletConsolidator.js, adapters/JSONStore.js, workers/ColdStorageWorker.js, workers/HelloWorldWorker.js) still uses require(...) / module.exports and throws ReferenceError: require is not defined in ES module scope when executed. package.json's files array still ships examples to npm, so this also breaks for downstream users copying examples as a starting point. Either convert these files to ESM to match the rest of the conversion, rename them to .cjs, or drop examples from files.
Source: claude
3. suggestion: Logger rewrite changes output streams and drops level prefix — silent behavior change
packages/wallet-lib/src/logger/index.js (line 1)
The previous winston-based logger wrote all levels to stdout via a single Console transport with a level: text prefix. The new implementation routes through native console.*, so warn/error now go to stderr while info/verbose/debug/silly go to stdout — observable change for downstream log scrapers and CI pipelines. The textual level prefix is also gone (winston previously preserved it; nothing replaces it). Neither is incorrect, but it's not noted in the PR's breaking-change list. Consider restoring a [level] prefix in fmt and calling out the stderr/stdout split.
Source: claude
4. suggestion: `loadDpp` default-import destructure and `.default ?? loadDpp` unwrap assume two different shapes
packages/wallet-lib/src/plugins/Workers/IdentitySyncWorker.js (line 1)
Line 2 destructures const { Identity } = loadDpp, which only works when loadDpp is the CJS module.exports namespace (today's behavior: Node's ESM↔CJS interop sets loadDpp to the entire exports object, so Identity resolves correctly and the previously claimed Identity === undefined bug does not actually trigger). But line 34 explicitly defends against the opposite interop shape with const load = loadDpp.default ?? loadDpp;. If wasm-dpp ever switches to native ESM with a single default export, the ?? fallback keeps load working, while the top-of-file destructure silently produces Identity === undefined and the failure surfaces only on the first Identity.fromBuffer(...) call. Make both lookups go through the same well-defined object — e.g. import * as wasmDpp from '@dashevo/wasm-dpp'; const { Identity } = wasmDpp; const load = wasmDpp.default ?? wasmDpp;.
Source: claude, codex
5. suggestion: Web example references deleted `dist/wallet-lib.min.js`
packages/wallet-lib/examples/web/usage.web.html (line 6)
webpack.config.js, the build:web / prepublishOnly scripts, and dist from files are all removed, but examples/web/usage.web.html still loads ../../dist/wallet-lib.min.js. The bundle no longer exists. Either delete examples/web/, replace it with a modern bundler example, or call out the loss of the browser bundle in the PR's breaking-change list.
Source: claude
6. nitpick: `events` devDependency is now unused
packages/wallet-lib/package.json (line 60)
All other browser polyfills (buffer, process, stream-browserify, etc.) were removed alongside webpack/karma. events was only needed as a polyfill for the deleted browser build and can be dropped for consistency.
Source: claude
7. nitpick: Missing trailing newline at end of file
packages/wallet-lib/src/plugins/Workers/IdentitySyncWorker.js (line 123)
Several converted files end without a trailing newline: IdentitySyncWorker.js, _loadStrategy.js, TransactionEstimator.js, coinSelections/index.js, coinSelections/strategies/index.js. POSIX tools and most lint/format configs expect a final newline. Mechanical fix.
Source: claude
Verifier-dropped findings
IdentitySyncWorkernow capturesIdentityfrom the loader function instead of the loaded DPP module — False positive — verified against Node's ESM↔CJS interop. wasm-dpp'sdist/index.jsis TypeScript-compiled CJS with__esModule = true,exports.default = loadDpp, and namedexports.Identity. When wallet-lib doesimport loadDpp from '@dashevo/wasm-dpp'from ESM, Node does NOT honor__esModule; the default import equals the entiremodule.exportsobject, soloadDpp.Identityis the class and the destructure resolves correctly. Confirmed empirically by simulating the dist shape and importing it from a.mjsfile:default.Identityis the class,default.defaultis the loader function. The consistency concern (what happens if wasm-dpp later switches to native ESM) is preserved as the suggestion-level finding above.
…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).
c96ad91 to
29640b4
Compare
|
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. |
Summary
Converts
@dashevo/wallet-libto pure ESM so it canimportthe ESMdapi-clientfrom PR 3. Drops webpack/karma and the browser-polyfill devDeps.This is PR 4 of 5 in the stacked series. Depends on #3679, #3680, and #3681.
What changes
wallet-lib
package.json:"type": "module",engines.node >= 18.18. Drops scriptsbuild:web,test:browsers*,prepublishOnly. Removes devDeps: allkarma-*,webpack, browser polyfills (buffer,crypto-browserify,https-browserify,os-browserify,path-browserify,process,stream-browserify,stream-http,string_decoder,url,util,assert,browserify-zlib,events,setimmediate). Removes depswinston(replaced with console-backed logger),node-inspect-extracted.src/,fixtures/, andtests/converted from CJS to ESM with.jsextensions on relative imports.src/logger/index.jsrewritten as a console-backed logger (matching the dapi-client pattern from PR 1).@dashevo/dapi-clientupdated to include.jsextensions (e.g.'@dashevo/dapi-client/lib/transport/ReconnectableStream.js').webpack.config.js,karma/,src/test/karma/..mocharc.ymlusesrequire:for the ESM bootstrap (works for this package's bootstrap shape under Mocha 11 + Node 18+).CJS-named-import conversions
Node ESM cannot statically enumerate named exports of CJS modules. So imports like
import { sortBy } from 'lodash';are replaced withimport lodash from 'lodash'; const { sortBy } = lodash;across all uses oflodash,crypto-js,@dashevo/dashcore-lib,@dashevo/wasm-dpp,@dashevo/grpc-common.Previously-silent CJS bugs that ESM strict mode catches
src/errors/index.js: addedUnknownStrategyto the named-exports list (the class file existed but was never re-exported).src/utils/coinSelections/strategies/index.js: added named exports forsimpleAscendingAccumulatorandsimpleDescendingAccumulator(previously only available via the defaultSTRATEGIESobject, butAccount.jswas importing them by name).src/types/DerivableKeyChain/DerivableKeyChain.spec.js: fixedtype='HDPublicKey'syntax error (an implicit-global assignment that worked in CJS sloppy mode but fails in ESM strict mode).Defensive wasm-dpp unwrap
src/plugins/Workers/IdentitySyncWorker.jsuses(loadDpp.default ?? loadDpp)()because wasm-dpp is CJS and the NodeNext default import sometimes resolves to the namespace object instead of the callable.Test plan
yarn workspace @dashevo/wallet-lib run test:unit— 377 passing, 2 pending, 0 failing ✅yarn workspace @dashevo/dapi-client run test:unit— 315 passing (no regression from PR 3)yarn workspace dash run test:unit— TS compile still fails on dapi-client deep imports (PR 5 fixes)Breaking changes
require('@dashevo/wallet-lib')breaks..jsextensions.Stack