Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ethers v5 - Dependency @ledgerhq/hw-transport-node-hid is crashing Netlify build #8

Closed
jurosh opened this issue Apr 24, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@jurosh
Copy link

jurosh commented Apr 24, 2020

For getting ledger work in Browser are those 2 dependencies needed:

 "@ledgerhq/hw-app-eth": "^5.14.0",
 "@ledgerhq/hw-transport-u2f": "^5.13.1", 

Ethers in version v5 is also using @ledgerhq/hw-transport-node-hid which does require native dependency libusb. This dependency is not available in Netlify environment:

Error:

prebuild-install WARN install libusb-1.0.so.0: cannot open shared object file: No such file or directory

Browser only requires @ledgerhq/hw-transport-u2f, so @ledgerhq/hw-transport-node-hid should not be needed. Ideal might be to split hardware wallets package to 3:

  1. with core functionality
  2. with browser provider using 1.
  3. with nodejs provider using 1.

This is just idea of possible solution, there are probably also other options. But removing this dependency would def help cause seems build of v5 (with HW wallets package) might crash in many environments configured for web builds.

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

I’m not sure what netlify is. Does it use rollup or some similar idea? There is likely some way to have it honour the “browser” field in a package.json, which may fix this issue?

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

(there are also builds in the hardware-wallet/dists folder which have already been built for the browser, which may help? Here is the rollup config that’s generate them.)

@jurosh
Copy link
Author

jurosh commented Apr 24, 2020

Problem is related to having @ledgerhq/hw-transport-node-hid dependency in package.json.

image

Build fails installing that. Netlify is CI service and it does not contains native dependencies required for building hw-transport-node-hid. But build failed for us also in our CircleCI setup.

Seems also configuring @ledgerhq/hw-transport-node-hid@5.3.0 as peerDependency would probably fix the build issue. That would just require for nodejs users to install it manually what should be OK though.

--

Just tried to manually remove @ledgerhq/hw-transport-node-hid dependency from our project's yarn.lock and now build works fine. So def problem is caused by this dependency which requires native - system libraries.

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

Maybe an optionalDependency makes more sense? I would rather it just work for node users too, without having to “do extra things”...

@jurosh
Copy link
Author

jurosh commented Apr 24, 2020

Maybe an optionalDependency makes more sense? I would rather it just work for node users too, without having to “do extra things”...

Wow, I haven't heard about optional deps yet. That seems as perfect solution.

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

Is there anyway for you to easily see if that solves your problem? I’ll prolly make that change regardless, but need to do some experimenting first to make sure it doesn’t break other things and have other things I need to get done first...

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

See: https://docs.npmjs.com/files/package.json#optionaldependencies

I don’t use yarn. I try to stick with the default and minimal tool chain, so I don’t force other to use things they don’t want to. :)

@jurosh
Copy link
Author

jurosh commented Apr 24, 2020

See: https://docs.npmjs.com/files/package.json#optionaldependencies

I don’t use yarn. I try to stick with the default and minimal tool chain, so I don’t force other to use things they don’t want to. :)

Yea, I found initially on yarn site, but then realized that it's also in official packagejson docs, so removed comment here - but you were very fast to reply 😄

Just tried build with optional dependency and deployed to npmjs here jurosh-hardware-wallets-optionaltest. The only change is moving hid here:

"optionalDependencies": {
    "@ledgerhq/hw-transport-node-hid": "5.13.1"
  },

Install in build works well now.

9:26:45 PM: info This module is OPTIONAL, you can safely ignore this error

Will play with ethers integration once build finish - but I suppose it will work fine as it should not require node-hid for integration inside browser.

Edit: Still seems like final step of build crashed.

11:07:20 PM: ./node_modules/@ledgerhq/hw-transport-node-hid-noevents/lib-es/TransportNodeHid.js
11:07:20 PM: Cannot find module: 'node-hid'. Make sure this package is installed.
11:07:20 PM: You can install this package by running: yarn add node-hid.

Trying to figure out where is that dependency being used. Maybe some dynamic import might fix this.

@ricmoo
Copy link
Member

ricmoo commented Apr 24, 2020

Awesome! I will need to modify the node part to use dynamic imports a bit (and throw a meaningful error if the hid library failed to install), but should be a fairly simple change.

@ricmoo ricmoo added the enhancement New feature or request label Apr 24, 2020
ricmoo referenced this issue in ethers-io/ethers.js Apr 25, 2020
@jurosh
Copy link
Author

jurosh commented Apr 27, 2020

Seems I still hit this error state when trying your latest changes. Output from build:

Failed to compile.

./node_modules/@ledgerhq/hw-transport-node-hid/lib-es/TransportNodeHid.js
Cannot find module: 'node-hid'. Make sure this package is installed.

You can install this package by running: yarn add node-hid.

That code is included by import { transports } from "./ledger-transport";

node-hid dependency is not installed cause it crashed, but parent dependency @ledgerhq/hw-transport-node-hid is still installed, so it does try to require it in code later on. But also without hw-transport-node-hid installed, it would still crash because webpack will try to lookup dependency for that require which is missing.

Btw. you should be able to reproduce with any webpack build (eg. very simple react app) -- on environment where node-hid is missing, eg. with Docker node CLI in web project directory: docker run --rm -v $(pwd):/build node /bin/sh -c 'cd build && yarn && yarn yourBuildCommand

--

To fix this, wouldn't be possible to refactor code (related to transport) a bit so it does require transport dependency to be installed from the outside - similar way how ledger is doing it - they probably faced same challenge.

Ledger way of doing importing:

import LedgerEth from "@ledgerhq/hw-app-eth";
import Transport from @ledgerhq/hw-transport-u2f"; // user pick transport
...
const transport = await TransportU2F.create();
const LedgerEth = new LedgerEth(transport);

In ethers.js it might be adding one new parameter for transport or something similar (simpler)...

import Transport from @ledgerhq/hw-transport-u2f"; // user pick ledger transport...
...
const transport = await TransportU2F.create();
new LedgerSigner(transport, ...)

--

Also I explored code a bit and saw there is code which should include proper ledger signer for browser...

"browser": {
    "./lib/ledger-transport.js": "./lib/browser-ledger-transport.js",
    "ethers": "./lib/browser-ethers.js"
  },

But Webpack (or Typescript compiler) doesn't seems to be picking this properly - at least not in latest code - cause it still take ledger for node which does crash.

Seems there are some discussions about this webpack/webpack#4674 So it might not be bulletproof.

Now tried to install "@ethersproject/hardware-wallets": "5.0.0-beta.5" and seems that version is not working in browser at all (not even is node-hid is available):

image

@ricmoo
Copy link
Member

ricmoo commented Apr 27, 2020

Have you looked at the latest code and tried that? Not what’s on npm. I think it might fix it.

Keep in mind you need to specify the mainFields to webpack to make it pick up the browser field in a package.json.

@jurosh
Copy link
Author

jurosh commented May 1, 2020

Have you looked at the latest code and tried that? Not what’s on npm. I think it might fix it.

Keep in mind you need to specify the mainFields to webpack to make it pick up the browser field in a package.json.

Yep, tried latest. Still seems to crash on

./node_modules/@ledgerhq/hw-transport-node-hid/lib-es/TransportNodeHid.js
Cannot find module: 'node-hid'. Make sure this package is installed.

Also added config.resolve.mainFields = ['browser', 'module', 'main']; to the webpack configuration but that doesn't seems to make any difference.

@ricmoo
Copy link
Member

ricmoo commented Nov 24, 2020

@jurosh Can you see if the latest version still causes issues for you?

The ESM build is now browser-friendly, so hopefully won't pull this package in on you. But I'm not sure exactly how netify works...

@ricmoo ricmoo transferred this issue from ethers-io/ethers.js Jan 18, 2023
@ricmoo ricmoo transferred this issue from ethers-io/ext-signer Jul 21, 2023
@ricmoo
Copy link
Member

ricmoo commented Jul 21, 2023

The new @ethers-ext/signer-ledger (this package) should now work. It doesn't bring in any native node packages, and instead expects the consumer to bring in the necessary (and compatible) Transports directly from the Ledger libraries, so hopefully this simplifies things for everyone.

Let me know if you have any issues with it though.

Thanks! :)

@ricmoo ricmoo closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants