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

feat: add rollup bundler, remove submodules #10

Closed
wants to merge 1 commit into from
Closed

Conversation

Polybius93
Copy link
Collaborator

This pull request adds Rollup dev dependencies to bundle the dist properly, enabling the option to import everything from one file. To achieve this, submodules were removed. Additionally, tiny-secpk256k1was replaced by @bitcoinlabs/tiny-secpk256k1. It also adds Buffer as a dependency, ensuring it is loadable in a browser context as a script.

sosaucily
sosaucily previously approved these changes Jun 21, 2024
Copy link
Contributor

@sosaucily sosaucily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, this is a great upgrade. Did you try to get the index.js file to be named something other than index.js? Or want to try that later? I suppose even if it's called index.js here, it can still be uploaded to the CDN with another name.

Also, just checking that this still works as an npm package?

@Polybius93
Copy link
Collaborator Author

Very cool, this is a great upgrade. Did you try to get the index.js file to be named something other than index.js? Or want to try that later? I suppose even if it's called index.js here, it can still be uploaded to the CDN with another name.

Also, just checking that this still works as an npm package?

Thanks! I didn't try to rename it, I didn't see any other example where the entry point is named differently. If you would like to have that modification, I can try it, but generally I don't see any reason to do that. I guess on the CDN we can call it however we want, but it won't be just a file name but a full link, so it won't confusing with other index.js files. I have tried it locally, it worked properly, but I haven't published it yet, so I didn't try it as a package, but I can before we merge it.

@sosaucily
Copy link
Contributor

We are using an API service now, instead of this package rolled-up, so this is no-longer needed

@sosaucily sosaucily closed this Oct 28, 2024
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