Workaround @ledgerhq/errors issue #15967#8147
Conversation
🦋 Changeset detectedLatest commit: f632aea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Introduces a CJS-import shim to work around @ledgerhq/errors ESM breakage (LedgerHQ/ledger-live#15967) and reduces Ledger package load impact by switching to lazy-loading where appropriate.
Changes:
- Add
src/internal/cjs-imports.ts(and a test-only equivalent) to load LedgerHQ dependencies viacreateRequire. - Update handler implementation and tests to consume LedgerHQ types/values from the new CJS-import shim.
- Lazy-load
LedgerHandlerin the network hook handler to avoid Ledger package load overhead when unused.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hardhat-ledger/src/internal/cjs-imports.ts | New CJS import shim for LedgerHQ deps to avoid broken ESM builds. |
| packages/hardhat-ledger/src/internal/handler.ts | Switch LedgerHQ imports/types to the shim and adjust constructor/instance typing. |
| packages/hardhat-ledger/src/internal/hook-handlers/network.ts | Lazy-load LedgerHandler to avoid loading Ledger deps unless needed. |
| packages/hardhat-ledger/test/internal/handler.ts | Update tests to import LedgerHQ error/types via the shim. |
| packages/hardhat-ledger/test/helpers/eth-mocked.ts | Update Eth mock typings to align with the shim. |
| packages/hardhat-ledger/test/helpers/tests-cjs-imports.ts | New test-only require-based import shim for mocks. |
| packages/hardhat-ledger/test/helpers/transport-node-hid-mock.ts | Update transport mock to use the test shim and base transport. |
| .changeset/gold-chefs-read.md | Patch changeset for the workaround release. |
| export function getEthMocked( | ||
| methodsConfig: MethodsConfig, | ||
| ): [typeof Eth.default, Map<string, MockCallState>] { | ||
| ): [typeof Eth, Map<string, MockCallState>] { | ||
| const calls = new Map<string, MockCallState>(); |
There was a problem hiding this comment.
Eth is imported with import type, but the function return type and cast use typeof Eth (a value type query), which won’t typecheck. If you want the constructor type, use the imported type directly (e.g. [Eth, ...] / as unknown as Eth), or switch to a value import.
| import type HwAppEthT from "@ledgerhq/hw-app-eth"; | ||
| import type HwTransportNodeHidT from "@ledgerhq/hw-transport-node-hid"; | ||
| import type { EIP712Message } from "@ledgerhq/types-live"; | ||
|
|
||
| import { createRequire } from "node:module"; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
|
|
||
| // Note: `typeof HwAppEthT` / `typeof HwTransportNodeHidT` are the module | ||
| // namespace types. Indexing them with ["default"] narrows to the exported | ||
| // class constructor, while `typeof HwAppEthT.default` collapses back to the | ||
| // namespace type. | ||
| type EthClass = (typeof HwAppEthT)["default"]; | ||
| type LedgerService = (typeof HwAppEthT)["ledgerService"]; | ||
| type TransportNodeHidClass = (typeof HwTransportNodeHidT)["default"]; |
There was a problem hiding this comment.
HwAppEthT and HwTransportNodeHidT are imported with import type ... from ..., but then used in typeof HwAppEthT / typeof HwTransportNodeHidT type queries. typeof requires a value symbol, so this will fail to typecheck. Use a module type query instead (e.g. typeof import("@ledgerhq/hw-app-eth")) or switch to a namespace type import (import type * as HwAppEthT from ...) so you can safely index the module exports.
| import type HwAppEthT from "@ledgerhq/hw-app-eth"; | |
| import type HwTransportNodeHidT from "@ledgerhq/hw-transport-node-hid"; | |
| import type { EIP712Message } from "@ledgerhq/types-live"; | |
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); | |
| // Note: `typeof HwAppEthT` / `typeof HwTransportNodeHidT` are the module | |
| // namespace types. Indexing them with ["default"] narrows to the exported | |
| // class constructor, while `typeof HwAppEthT.default` collapses back to the | |
| // namespace type. | |
| type EthClass = (typeof HwAppEthT)["default"]; | |
| type LedgerService = (typeof HwAppEthT)["ledgerService"]; | |
| type TransportNodeHidClass = (typeof HwTransportNodeHidT)["default"]; | |
| import type { EIP712Message } from "@ledgerhq/types-live"; | |
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); | |
| // Note: module type queries give us the module namespace types. Indexing them | |
| // with ["default"] narrows to the exported class constructor. | |
| type EthClass = (typeof import("@ledgerhq/hw-app-eth"))["default"]; | |
| type LedgerService = (typeof import("@ledgerhq/hw-app-eth"))["ledgerService"]; | |
| type TransportNodeHidClass = | |
| (typeof import("@ledgerhq/hw-transport-node-hid"))["default"]; |
| import type TransportT from "@ledgerhq/hw-transport"; | ||
|
|
||
| import { createRequire } from "node:module"; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
|
|
||
| type BaseTransportClass = (typeof TransportT)["default"]; |
There was a problem hiding this comment.
TransportT is imported with import type, but then referenced in a typeof TransportT type query. This will fail to typecheck because typeof requires a value symbol. Consider replacing this with a module type query like type BaseTransportClass = (typeof import("@ledgerhq/hw-transport"))["default"]; (and remove the import type TransportT ...).
| import type TransportT from "@ledgerhq/hw-transport"; | |
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); | |
| type BaseTransportClass = (typeof TransportT)["default"]; | |
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); | |
| type BaseTransportClass = (typeof import("@ledgerhq/hw-transport"))["default"]; |
| describe("LedgerHandler", () => { | ||
| let ethereumMockedProvider: EthereumMockedProvider; | ||
| let eth: typeof Eth.default; | ||
| let eth: typeof Eth; |
There was a problem hiding this comment.
Eth is imported with import type, but this line uses typeof Eth (a value type query). That combination won’t typecheck. Either import Eth as a value, or keep it type-only and use the imported type directly (e.g. let eth: Eth).
| let eth: typeof Eth; | |
| let eth: Eth; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| // The ledger packages have been problematic in the past, leading to errors | ||
| // and slowdowns, even when not being used, so we lazy load them now. | ||
| let LedgerHandler: typeof LedgerHandlerT | undefined; |
There was a problem hiding this comment.
LedgerHandlerT is imported with import type, so it’s a type-only symbol. Using typeof LedgerHandlerT here will not type-check (TypeScript typeof in a type position requires a value). Define a constructor type via typeof import("../handler.js").LedgerHandler (or a local type LedgerHandlerConstructor = typeof import("../handler.js")["LedgerHandler"]) and use that for the lazy-loaded variable instead.
| // The ledger packages have been problematic in the past, leading to errors | |
| // and slowdowns, even when not being used, so we lazy load them now. | |
| let LedgerHandler: typeof LedgerHandlerT | undefined; | |
| type LedgerHandlerConstructor = typeof import("../handler.js")["LedgerHandler"]; | |
| // The ledger packages have been problematic in the past, leading to errors | |
| // and slowdowns, even when not being used, so we lazy load them now. | |
| let LedgerHandler: LedgerHandlerConstructor | undefined; |
| const BaseTransport: BaseTransportClass = hwTransport.default; | ||
|
|
||
| export { BaseTransport }; | ||
| export { TransportNodeHidT }; |
There was a problem hiding this comment.
This is a type-only import, but it’s being re-exported as a value (export { TransportNodeHidT }). This should be a type export (export type { TransportNodeHidT }) or the file won’t compile under TypeScript’s type-only import rules.
| export { TransportNodeHidT }; | |
| export type { TransportNodeHidT }; |
| * other ledger packages have different version ranges, and we can end up with | ||
| * multiple installations and still hit the error. | ||
| * | ||
| * The workaround consists on importing the CJS version of the package, by using |
There was a problem hiding this comment.
Minor grammar: “consists on importing” should be “consists of importing”.
| * The workaround consists on importing the CJS version of the package, by using | |
| * The workaround consists of importing the CJS version of the package, by using |
|
Let's ignore copilot here. This workaround is ugly, but we need it |
Link to issue: LedgerHQ/ledger-live#15967