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

Chrome extension compatibility #10

Open
wants to merge 4 commits into
base: s-x-compat
Choose a base branch
from

Conversation

caseykennedy
Copy link

@caseykennedy caseykennedy commented Oct 20, 2021

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
Copy link
Member

What's the reasoning behind changing the import syntax?

@pinheadmz
Copy link
Member

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
Copy link
Author

caseykennedy commented Jan 11, 2022

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
Copy link
Author

caseykennedy commented Jan 13, 2022

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
Copy link
Member

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

@caseykennedy
Copy link
Author

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

Yes, we are using webpack to bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants