handshake-org / hsd-ledger

Javascript client library for the Handshake Ledger application
Other
18 stars 7 forks source link

Chrome extension compatibility #10

Open caseykennedy opened 3 years ago

caseykennedy commented 3 years ago

This PR uses direct instead of dynamic imports and creates a usb-browser file in order to import busb-browser. I am not sure if the new usb-browser file is the best way to accomplish the import...

pinheadmz commented 2 years ago

What's the reasoning behind changing the import syntax?

pinheadmz commented 2 years ago

I think there must be a better way than copying the entire USB.js file just to change one line ;-)

We already have this dynamic require logic for Speculos testing:

if (process.env.SPECULOS === 'true')
  busb = require('./speculos');
else
  busb = require('busb');

can you add an environment variable for browser here?

caseykennedy commented 2 years ago

What's the reasoning behind changing the import syntax?

It throws 'cannot find' errors when importing into chrome extensions. I do not have the specific error any longer but will recreate and share.

I thought there could be a better way too! Thank you for pointing this out. Since opening this PR some things have changed with the bob/ledger integration and it no longer requires busb-browser. Will update the PR asap.

Appreciate your eyes on this!

caseykennedy commented 2 years ago

Thank you for your patience — just to follow up:

I removed the usb-browser file as well as the 'busb-browser' import as it is no longer necessary for the integration I am working on. If you would still like me to include env.BROWSER I would be happy to write in.

As far as removing the dynamic imports, the error is:

Cannot find module './primitives/address'

and so on down the line. Changing the import structure fixes the issue.

Just pushed the updated commit, please let me know if it looks good.

Thank you!!

pinheadmz commented 2 years ago

What actually is returning that error? I mean, are you bundling with webpack or something like that?

caseykennedy commented 2 years ago

What actually is returning that error? I mean, are you bundling with webpack or something like that?

Yes, we are using webpack to bundle.