-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Comments
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? |
Problem is related to having Build fails installing that. Netlify is CI service and it does not contains native dependencies required for building Seems also configuring -- Just tried to manually remove |
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. |
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... |
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 Just tried build with optional dependency and deployed to npmjs here
Install in build works well now.
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.
Trying to figure out where is that dependency being used. Maybe some dynamic import might fix this. |
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. |
Seems I still hit this error state when trying your latest changes. Output from build:
That code is included by
Btw. you should be able to reproduce with any webpack build (eg. very simple react app) -- on environment where -- 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:
In ethers.js it might be adding one new parameter for transport or something similar (simpler)...
-- Also I explored code a bit and saw there is code which should include proper ledger signer for browser...
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 |
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
Also added |
@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... |
The new Let me know if you have any issues with it though. Thanks! :) |
For getting ledger work in Browser are those 2 dependencies needed:
Ethers in version v5 is also using
@ledgerhq/hw-transport-node-hid
which does require native dependencylibusb
. This dependency is not available in Netlify environment:Error:
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: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.
The text was updated successfully, but these errors were encountered: