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

Issues bundling for browser #1729

Closed
TJKoury opened this issue Oct 11, 2021 · 10 comments
Closed

Issues bundling for browser #1729

TJKoury opened this issue Oct 11, 2021 · 10 comments

Comments

@TJKoury
Copy link

TJKoury commented Oct 11, 2021

I have been using Vite recently, and since it does not pre-bundle or use CommonJS there have been numerous issues integrating this library.

I tried getting around this by using Webpack/Rollup/Browserify to pre-create a bundle, but no matter what config I try to use there are dependencies that are not included or error out.

Changing the tsconfig to use esnext doesn't seem to work either, as require statements for several libraries are still included (such as randombytes, typeforce, wif, tiny-secp256k1, among others), and do not resolve correctly using any bundler even when using plugins that handle CommonJS files and use node resolution.

My next step is to hand-edit the resultant code in the src directory to force convert to es6, but before I do that I figured I would drop this issue and see if there is something fundamental that I a missing.

@junderw
Copy link
Member

junderw commented Oct 11, 2021

I personally use browserify with the current latest versions and it works fine.

Can you give more concrete examples of what you tried.

We only support browserify, so anything besides that is not supported and therefore not an issue.

But I would like to know how you attempted to build the browserify bundle.

@TJKoury
Copy link
Author

TJKoury commented Oct 11, 2021

I tried the incantation from this thread, leaving out bigi.

Getting error error: Unterminated string literal when importing it in the browser.

@TJKoury
Copy link
Author

TJKoury commented Oct 11, 2021

Looks like I might have found a solution running the result through rollup. I'll keep testing it to see.

Having read the other threads on this issue, I see the sentiment of "if you do not understand how to bundle, you have no business integrating this library" as the argument against having a bundled version available, or even the commands to bundle in the package.json.

As a very grateful non-contributor, here's my 2 cents:

If there is no approved build process, folks will end up doing stupid things to integrate this library, opening up all kinds of security and compatibility issues. I am fairly experienced and this has been one of the harder libraries to integrate into new build systems.

At the very least, a native es6 version would go a long way to fixing this issue, and I would certainly be willing to contribute to creating one.

@TJKoury TJKoury closed this as completed Oct 11, 2021
@junderw
Copy link
Member

junderw commented Oct 11, 2021

Every time someone brings up bundling issues, I try to reproduce and fail. (ie. I always succeed with bundling with zero effort)

I even tried to wedge my normal commands into a fresh node v14 docker container to see if it still works and it does.

This will spit out a minified bundle and a simple index.html that loads it so I can try it out in the browser console.

Everything works fine.

See the docker command (with a script passed as a long string arg) below.

#!/usr/bin/env bash
export FOLDER="$(pwd)/bitcoinjstmpfolder"; docker run -v "$FOLDER":/data -it node:14 bash -c "
  # Install deps
  echo 'Installing bundler...'
  npm install -g browserify uglify-js >/dev/null 2>&1
  mkdir /app
  cd /app
  npm init -y >/dev/null 2>&1
  echo \"const bitcoinjs = require('bitcoinjs-lib')
  bitcoinjs.Buffer = require('buffer').Buffer
  module.exports = bitcoinjs\" > index.js
  echo 'Installing bitcoinjs...'
  npm install --save bitcoinjs-lib >/dev/null 2>&1

  # Build
  echo 'Building browser bundle...'
  browserify -r . --standalone bitcoinjs | \\
  uglifyjs -c --mangle reserved=['BigInteger','ECPair','Point'] \\
  -o bitcoinjs.min.js

  # Move
  echo 'Moving to $FOLDER/bitcoinjs.min.js ...'
  chmod 777 bitcoinjs.min.js
  mv ./bitcoinjs.min.js /data

  # Make test html file
  echo '<html><script src=\"./bitcoinjs.min.js\"></script></html>' > /data/index.html
  chmod 777 /data/index.html
"

@TJKoury
Copy link
Author

TJKoury commented Oct 11, 2021

Tested, this works for platform (Linux 5.11.0-7614-generic x86_64):

  1. Node 16.3.0
  2. Chrome Version 93.0.4577.63 (Official Build) (64-bit) in a <script> tag
  3. Vite (2.6.7) after transforming separately using @rollup/plugin-commonjs

Fails (error: Unterminated string literal):

  1. Imported via vite (2.6.7) using vite-plugin-commonjs (1.0.0)
  2. Imported via vite (2.6.7) using @rollup/plugin-commonjs

@TJKoury
Copy link
Author

TJKoury commented Oct 11, 2021

It does sound like a vite issue, though I have not tried it with enough build systems to confirm.

@junderw
Copy link
Member

junderw commented Oct 11, 2021

If there was actually an "Unterminated string literal" one would expect it to not even load into any browser at all.

This is definitely an issue with vite or some other tooling you have in between.

If you can figure out a simple change to the library that will allow us to work with vite, I'll merge it, but tbh any major work beyond that will most likely be wasted since we're going to move towards WASM in the next major release anyways.

@TJKoury
Copy link
Author

TJKoury commented Oct 11, 2021

Are you going to take the code from Bitcoin core for the wasm build? Sounds like a great plan, and will integrate nicely with some wasm work we are doing as well.

@TJKoury
Copy link
Author

TJKoury commented Oct 12, 2021

Turns out it's a Rollup specific issue in Vite, due to circular dependencies in readable-stream.

@TJKoury
Copy link
Author

TJKoury commented Feb 18, 2022

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

No branches or pull requests

2 participants