chore(dapi-client,dapi-grpc): cleanup — drop unused deps, inline winston/fetch/promisify shims#3674
Conversation
…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').
📝 WalkthroughWalkthroughThis PR eliminates Node.js-specific dependencies from ChangesBrowser Compatibility Initiative
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/dapi-grpc/clients/core/v0/web/CorePromiseClient.jsOops! Something went wrong! :( ESLint: 9.39.4 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/js-dapi-client/lib/logger/index.jsOops! Something went wrong! :( ESLint: 9.39.4 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by .pnp.cjsOops! Something went wrong! :( ESLint: 9.39.4 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/js-dapi-client/package.json (1)
74-76: ⚡ Quick winConsider loosening the Node.js engine requirement to
>= 18.0.0or>= 18.2.0.The codebase uses
fetch()(available in Node 18.0+, stable in 18.2+) andBuffer(available since Node 0.1+). No APIs requiring Node 18.18+ were found, and no dependencies explicitly require that version. The presence of asetImmediatepolyfill in devDependencies further suggests the package was designed to support older Node versions. Unless there's a specific stability concern with Node 18.0–18.17 or an undocumented dependency requirement, the engine constraint can safely be lowered to accommodate a broader range of Node 18 users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js-dapi-client/package.json` around lines 74 - 76, The engines field currently pins Node to ">=18.18" which is stricter than necessary; update the "engines" entry in package.json (the "node" key) to a looser requirement such as ">=18.2.0" or ">=18.0.0" depending on desired support (use ">=18.2.0" if you want to require the stable fetch implementation), and ensure package.json's "engines" object reflects the new version range.packages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.js (1)
6-9: ⚖️ Poor tradeoffConsider extracting the promisify shim to a shared utility.
The
promisifyimplementation is duplicated identically inCorePromiseClient.js(lines 4-7). While both implementations are correct, extracting to a shared module would follow DRY principles.However, given that these files appear to be generated (based on the codegen template comment) and the PR goal is minimal invasiveness, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.js` around lines 6 - 9, The file duplicates a small promisify shim also present in CorePromiseClient.js; extract this function into a shared utility (e.g., a new module exporting promisify) and update PlatformPromiseClient.js and CorePromiseClient.js to import and use that shared promisify export instead of redefining it so the duplicate implementation is removed and both files call the same exported function.packages/js-dapi-client/lib/logger/index.js (1)
1-7: 💤 Low valueUse camelCase for the module constants.
LOG_LEVELandLEVELSshould follow the repo's JS naming convention (logLevel,levels).As per coding guidelines, "Use camelCase for variables and functions in JS/TS".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js-dapi-client/lib/logger/index.js` around lines 1 - 7, Rename the module constants to camelCase: change LOG_LEVEL to logLevel and LEVELS to levels (leave cache as is or rename to camelCase if desired), then update every usage and export/import in this file to reference logLevel and levels (including the initialization expression that reads process.env.LOG_LEVEL and the object keys). Also update any external consumers/imports that import LOG_LEVEL or LEVELS to use the new names (or provide backward-compatible aliases in this module while updating references), and run tests to ensure no unresolved references remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.js`:
- Around line 6-9: The file duplicates a small promisify shim also present in
CorePromiseClient.js; extract this function into a shared utility (e.g., a new
module exporting promisify) and update PlatformPromiseClient.js and
CorePromiseClient.js to import and use that shared promisify export instead of
redefining it so the duplicate implementation is removed and both files call the
same exported function.
In `@packages/js-dapi-client/lib/logger/index.js`:
- Around line 1-7: Rename the module constants to camelCase: change LOG_LEVEL to
logLevel and LEVELS to levels (leave cache as is or rename to camelCase if
desired), then update every usage and export/import in this file to reference
logLevel and levels (including the initialization expression that reads
process.env.LOG_LEVEL and the object keys). Also update any external
consumers/imports that import LOG_LEVEL or LEVELS to use the new names (or
provide backward-compatible aliases in this module while updating references),
and run tests to ensure no unresolved references remain.
In `@packages/js-dapi-client/package.json`:
- Around line 74-76: The engines field currently pins Node to ">=18.18" which is
stricter than necessary; update the "engines" entry in package.json (the "node"
key) to a looser requirement such as ">=18.2.0" or ">=18.0.0" depending on
desired support (use ">=18.2.0" if you want to require the stable fetch
implementation), and ensure package.json's "engines" object reflects the new
version range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a698a9b-0b2a-47c0-996d-4e24da3e1133
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.pnp.cjspackages/dapi-grpc/clients/core/v0/web/CorePromiseClient.jspackages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.jspackages/js-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.jspackages/js-dapi-client/lib/index.jspackages/js-dapi-client/lib/logger/index.jspackages/js-dapi-client/lib/test/bootstrap.jspackages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.jspackages/js-dapi-client/package.jsonpackages/js-dapi-client/polyfills/fetch-polyfill.js
💤 Files with no reviewable changes (4)
- packages/js-dapi-client/lib/index.js
- packages/js-dapi-client/lib/test/bootstrap.js
- packages/js-dapi-client/polyfills/fetch-polyfill.js
- packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent blocking issues: the karma test bootstrap still require()s a polyfill file deleted in this PR (browser test suite will fail at module resolution), and the JSON-RPC transport silently ignores the selfSigned flag while ListDAPIAddressProvider still actively sets allowSelfSignedCertificate = true for regtest/localhost addresses, breaking local Dashmate workflows. Two non-blocking follow-ups around dead test coverage and a leftover devDependency.
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s)
blocking: Karma bootstrap requires deleted fetch-polyfill
packages/js-dapi-client/lib/test/karma/bootstrap.js (line 1)
Line 1 still has require('../../../polyfills/fetch-polyfill');, but this PR deletes packages/js-dapi-client/polyfills/fetch-polyfill.js (the polyfills/ directory no longer exists in the worktree). karma.conf.js loads lib/test/karma/loader.js, which in turn requires this bootstrap, so webpack bundling for yarn workspace @dashevo/dapi-client test:browsers will fail at module resolution before any spec runs. Since package.json defines test as test:coverage && test:browsers, the package-level yarn test is broken on this SHA. With Node 18+ providing global fetch and modern browsers having it natively, the require can simply be dropped.
require('setimmediate');
blocking: `selfSigned` is now silently ignored despite still being produced for regtest
packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js (line 14)
requestJsonRpc still accepts selfSigned and embeds it in requestInfo, but the call to fetch(url, requestOptions) at line 49 no longer attaches any TLS override. This is not just dead API surface: lib/dapiAddressProvider/ListDAPIAddressProvider.js:31-41 actively rewrites local/regtest masternodes to https://127.0.0.1 and sets allowSelfSignedCertificate = true, which JsonRpcTransport continues to forward into this function. As a result, core JSON-RPC calls (getBestBlockHash, getBlockHash, etc.) against local Dashmate nodes will now fail TLS verification with no recovery path. Either restore an opt-in mechanism (e.g., expose a custom-fetch hook or document NODE_TLS_REJECT_UNAUTHORIZED=0) and stop forwarding a flag that does nothing, or remove the regtest self-signed branch in ListDAPIAddressProvider so callers aren't left with a broken local workflow. At minimum, update the JSDoc and DAPIAddress.js docs to note the parameter is inert.
suggestion: Self-signed cert test is now a tautology
packages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js (line 76)
should make https rpc request with self-signed certificate and return result only asserts expect(result).to.equal('passed') against a stubbed fetch. The production branch it used to cover (new https.Agent({ rejectUnauthorized: false }) injected onto requestOptions.agent) was removed in this PR, but the test still passes because nothing about selfSigned is asserted on the fetch call. As written it provides zero coverage of the documented selfSigned parameter. Delete it or rewrite it to assert that selfSigned=true is now a no-op (e.g., fetch is invoked with no agent property), so future regressions in this path are caught.
nitpick: `setimmediate` still in devDependencies after PR claims removal
packages/js-dapi-client/package.json (line 64)
The PR description lists setimmediate under removed deps, but "setimmediate": "^1.0.5" remains in devDependencies because lib/test/karma/bootstrap.js:2 still does require('setimmediate'). Either remove the devDependency together with the karma require (browsers in scope here have setImmediate polyfilled via webpack's process shim or it's unused) or correct the PR description. Mostly a description/diff mismatch, not a runtime defect.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/js-dapi-client/lib/test/karma/bootstrap.js`:1-1: Karma bootstrap requires deleted fetch-polyfill
Line 1 still has `require('../../../polyfills/fetch-polyfill');`, but this PR deletes `packages/js-dapi-client/polyfills/fetch-polyfill.js` (the `polyfills/` directory no longer exists in the worktree). `karma.conf.js` loads `lib/test/karma/loader.js`, which in turn requires this bootstrap, so webpack bundling for `yarn workspace @dashevo/dapi-client test:browsers` will fail at module resolution before any spec runs. Since `package.json` defines `test` as `test:coverage && test:browsers`, the package-level `yarn test` is broken on this SHA. With Node 18+ providing global `fetch` and modern browsers having it natively, the require can simply be dropped.
- [BLOCKING] In `packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js`:14-49: `selfSigned` is now silently ignored despite still being produced for regtest
`requestJsonRpc` still accepts `selfSigned` and embeds it in `requestInfo`, but the call to `fetch(url, requestOptions)` at line 49 no longer attaches any TLS override. This is not just dead API surface: `lib/dapiAddressProvider/ListDAPIAddressProvider.js:31-41` actively rewrites local/regtest masternodes to `https://127.0.0.1` and sets `allowSelfSignedCertificate = true`, which `JsonRpcTransport` continues to forward into this function. As a result, core JSON-RPC calls (`getBestBlockHash`, `getBlockHash`, etc.) against local Dashmate nodes will now fail TLS verification with no recovery path. Either restore an opt-in mechanism (e.g., expose a custom-fetch hook or document `NODE_TLS_REJECT_UNAUTHORIZED=0`) and stop forwarding a flag that does nothing, or remove the regtest self-signed branch in `ListDAPIAddressProvider` so callers aren't left with a broken local workflow. At minimum, update the JSDoc and `DAPIAddress.js` docs to note the parameter is inert.
- [SUGGESTION] In `packages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js`:76-99: Self-signed cert test is now a tautology
`should make https rpc request with self-signed certificate and return result` only asserts `expect(result).to.equal('passed')` against a stubbed `fetch`. The production branch it used to cover (`new https.Agent({ rejectUnauthorized: false })` injected onto `requestOptions.agent`) was removed in this PR, but the test still passes because nothing about `selfSigned` is asserted on the fetch call. As written it provides zero coverage of the documented `selfSigned` parameter. Delete it or rewrite it to assert that `selfSigned=true` is now a no-op (e.g., `fetch` is invoked with no `agent` property), so future regressions in this path are caught.
- [NITPICK] In `packages/js-dapi-client/package.json`:64-64: `setimmediate` still in devDependencies after PR claims removal
The PR description lists `setimmediate` under removed deps, but `"setimmediate": "^1.0.5"` remains in `devDependencies` because `lib/test/karma/bootstrap.js:2` still does `require('setimmediate')`. Either remove the devDependency together with the karma require (browsers in scope here have `setImmediate` polyfilled via webpack's process shim or it's unused) or correct the PR description. Mostly a description/diff mismatch, not a runtime defect.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Closing — reopening as upstream→upstream so CI runs against secrets/runners. New PR link will be added shortly. |
|
Reopened as #3679 (upstream→upstream so CI runs against repo secrets/runners). |
2 participants
Summary
Non-breaking cleanup pass on
@dashevo/dapi-clientand the browser path of@dashevo/dapi-grpc. Package stays CJS, public API unchanged, no consumer changes required.This is PR 1 of 5 in a stacked series that incrementally makes
dapi-clientbrowser-native and ESM. Each PR is independently shippable and leaves the monorepo green.What changes
@dashevo/dapi-clientwinstonwith a ~30-line console-backed logger that preserves the same API (.error/.warn/.info/.verbose/.debug/.silly/.getForId).node-fetchand thefetch-polyfill(Node 18+ has globalfetch).setimmediate(Node 18+ has it globally).https.Agent({rejectUnauthorized: false})self-signed-TLS branch fromrequestJsonRpc. Was Node-only and can't be made browser-portable. Consumers needing this must setNODE_TLS_REJECT_UNAUTHORIZED=0or pass a customfetchat the app layer.lodash/sampleinListDAPIAddressProvider(one call site).engines.node >= 18.18.winston,node-fetch,lodash,bs58,node-inspect-extracted,setimmediate.@dashevo/dapi-grpcpromisifyshim inclients/core/v0/web/CorePromiseClient.jsandclients/platform/v0/web/PlatformPromiseClient.js, removing therequire('util')from the browser bundle path. Both files document the shim so a future codegen regen does not silently reintroducerequire('util').Test plan
yarn workspace @dashevo/dapi-client run test:unit— 315 passingyarn workspace @dashevo/wallet-lib run test:unit— 377 passing (no regression in downstream consumer)yarn workspace dash run test:unit— 60 passing (no regression in js-dash-sdk)Breaking changes
requestJsonRpc({selfSigned: true})no longer adds a permissivehttps.Agent. Workaround:NODE_TLS_REJECT_UNAUTHORIZED=0or customfetch.engines.node >= 18.18(was unspecified; older Node lacks globalfetch).Stack
Buffer→Uint8Arrayin dapi-client public surface@dashevo/dapi-client→ ESM (dual-format CJS+ESM)@dashevo/wallet-lib→ ESM@dashevo/js-dash-sdk+@dashevo/platform-test-suite→ ESMSummary by CodeRabbit
Chores
Bug Fixes