Expose API dependencies by default

[dcousens]

Continuation of the discussion in #228.

There seems to be a collective of users that don't want to go down the browserify path, and instead prefer the all-in-1 bundle approach (for better or worse).

This pull request would provide the latter.

On the surface, this is a terrible decision in avoiding future problems where we do intend to change the underlying BigInteger implementation, and remove CryptoJS as a dependency completely.
Its a step backwards in re-usable software, but its a massive convenience to those who just want to plug and play with bitcoin in Javascript.

Thoughts? Can we somehow have both? Maybe a separate repository that bundles all this together that we can point them at?

[coveralls]

Coverage remained the same when pulling 884c190 on dcousens:expose into df743e5 on bitcoinjs:master.

[Shic1983]

its a massive convenience to those who just want to plug and play with bitcoin in Javascript.

+1

Maybe a separate repository that bundles all this together that we can point them at?

Yes please

[kyledrake]

This doesn't eliminate browserify, it just provides all the dependencies. You still need to use browserify to use the library properly in the browser, otherwise it will only work in node.js.

See my comments on #228. I am opposed to de-coupling browserify from the code, and it's basically impossible to use it in the browser without using browserify anyways. If developers need these external dependencies, they should include them in their project directly.

[kyledrake]

Possible compromise:

We expose the dependencies, but in a way that's separated from the rest of the code.

So, instead of doing bitcoinjs.bigi ala bitcoinjs.Transaction, we would do something like bitcoinjs.deps.bigi.

Or more correctly, bitcoinjs.dontusethisitwillprobablybreak.bigi. But if users are willing to take the risk, that's a way to do it and highlight the dependency.

The thing is, at least one of these is guaranteed to eventually break, so why bother?

[jprichardson]

The thing is, at least one of these is guaranteed to eventually break, so why bother?

I'm with you on this. At first I was in favor of the idea, but exposing dependencies (that are out of scope) seems like a bad way to tie yourself to legacy code. It also sets up expectations for consumers of the API. Now, it is true that this problem can be solved by proper adherence to semver, but what's the point if we know that these dependencies will change. Especially, bigi and ecurve. @dcousens and I have been discussing a port of ecurve to a faster big integer library. It won't happen soon (this month or so), but maybe within three months. The point is, it'll happen and there's not much of a reason to expose consumers to unnecessary change. bigi is definitely out of scope in this library, a possibly the same case could be made for ecurve.

[weilu]

Like I said to @dcousens on IRC, I'm on the fence. I used to strongly believe that what we are doing now (not exposing dependencies) is the way to go. But by now there are at least 3 people who have requested the opposite because they want to use bitcoinjs-lib as a bundle in browser without browserify. I hate to stop devs from using our library because of this. And code wise, it doesn't take us much effort to expose our dependencies.

The downside of exposing our dependencies is the interface change when we swap/upgrade our underlining library. So going forward if we are using semver, when our dependencies bump their major version, we need to bump ours.

[dcousens]

Alternatively we could provide a secondary compilation target.

npm run-script compile-bundle

Which compiles the following (or something similar) using browserify:

module.exports = {
  base58: require('bs58'),
  bigi: require('bigi'),
  bitcoinjs: require('bitcoinjs-lib'),
  cryptojs: require('crypto-js'),
  ecurve: require('ecurve')
}

That way in the event of an API change (say we switch ecc implementations), it is clear that only our API will break, and not their use of the above modules.

[Shic1983]

I just wanted to chime in here, because, being a complete browserify idiot, and not knowing anything about js package development / management, I have basically been stuck as fook.

Now, I did as @dcousens said;
I added a bundle.js which included the additional exports, I then added a compile-bundle command to the package.json .. and now I have access to utilize all the existing external stuff aswell.. I know it seems really stupid now, but not knowing shit about browserify, it left me stumped...

So on this basis, I would recommend including the compile-bundle one in the package.json . .. simply so people can either try the just compile on itself, or if they need access to the full feature set.. then they can run the compile bundle... and from what I can tell.. it doesn't increase file size at all.. only makes the inaccessible... accessible.. so makes sense..

[weilu]

Can we get away with simply documenting in README how one could create their own secondary compilation target?

[kyledrake]

I'm okay with the compile-bundle compromise, if it was called 'compile-with-internal-deps' or something, and has a warning placed next to it (and in the README) that the dependencies are subject to change in newer versions.

[dcousens]

Added compile-bundle target and bundle.js.

[coveralls]

Coverage remained the same when pulling e05095b on dcousens:expose into dcc9ddf on bitcoinjs:master.

[weilu]

I agree with @kyledrake that compile-bundle probably isn't the best name. compile-with-dependencies?

Also can we avoid repeating ourselves by making bundle.js something like

var bitcoin = require('./src/index')

var exports = {
  base58: require('bs58'),
  BigInteger: require('bigi'),
  CryptoJS: require('crypto-js'),
  ec: require('ecurve'),
  securerandom: require('secure-random')
}

for(var key in bitcoin) {
  exports[key] = bitcoin[key]
}

module.exports = exports

[coveralls]

Coverage remained the same when pulling a536b9d on dcousens:expose into 656de37 on bitcoinjs:master.

[weilu]

@dcousens thoughts on the task name?

[dcousens]

Sounds good to me, @weilu thoughts on the corresponding file name bundle.js?

[weilu]

index+.js? indexpuls.js? bitcoinjsdelux.js? jk. can't think of anything better. Why isn't it in the src folder?

[kyledrake]

move bundle.js to src and I'm +1!

[dcousens]

Despite everything that has been discussed to this point.

I vote we expose the dependencies in favour of forcing users to use the version we use. It sucks, but instanceof and the way npm can't always de-dupe is going to kill developer productivity like in #255.

The other option is "peerDependencies", but that would stop users using their own version of the dependencies in their projects completely, not just when interacting with our API.

[dcousens]

At this point, the following code will throw an exception about instanceof failing in ECPubKey when ecurve is not the same version between the users own package.json and ours.

var BigInteger = require('bigi')
var ecurve =require('ecurve')
var secp256k1 = ecurve.getCurveByName('secp256k1')

var d = new BigInteger('555555')
var Q = secp256k1.G.multiply(d)

// FIXME: Throws
var pubKey = new Bitcoin.ECPubKey(Q)

This is exactly how it should look as the 'ideal' demonstration of how to use our API, and yet it fails unless the user uses identical dependencies to us.

[weilu]

As mentioned in #255 (comment), my take is that

At this point, exposing our dependencies seems like the simplest and least harmful solution to the instanceof problem.

[jprichardson]

If you want to adhere to semver, exposing dependencies is a bad idea as any incompatible change would force bitcoinjs-lib to bump its major version. Assuming you want to adhere to semver as it's kinda expected in the Node.js world. Get rid of instanceof as it's harmful. In cases of BigInteger, use isBigInteger method. In all other cases you can use the .constructor.name === what you want it to be.

[jprichardson]

An example of the .constructor.name solution:

var pub = new ECPubKey(...)

function someMethod(key) {
  if (key.constructor.name !== 'ECPubKey') throw new Error('Not ECPubKey')
}

This will work for different versions of ECPubKey.

[jprichardson]

Per IRC discussion with @weilu I've changed my mind on exposing the dependencies. It makes sense so consumers can freely use them and not worry about incompatible changes amongst versions; even worse, if there was an improper calculation leading to a silent error.

[glorat]

Hey folks,

Just to weigh in with some opinions and more options... If I were you guys

• I would not want to expose my dependencies
• I would not want to be overly exposed to random changes in my dependencies breaking me either

There are potential options in between that I've seen for other languages that I guess could be adapted to javascript

• Require yourself to be initialised before use: var x=require('bitcoinjs-lib'); x.init(require('bigi'), require('crypto-js')...). init can then assert version numbers, capability, self test, whatever you like. A bit like dependency injection. This allows your clients to choose your dependencies while still giving yourself protection
• Ensure your library never returns instances of your dependent libraries. Where needed, wrap it. BJSBigInteger, BJSECurve. Be totally explicit you are not allowing an externally created BigInteger or Point to be passed into your library. Net effect is that you are refusing externally created objects to be passed to you. You expose factories for anything that might get passed to you. So this is like exposing your dependencies... but only the ones actually used by your library so people want end up building GPP stuff with BitCoinJS.ECurve.

Or heck, just lose the isInstanceOf and caveat emptor on the use of your library. Self-test asserts on init anyway.

I don't have a Right Answer :)

[dcousens]

Thanks for the comments @glorat :)

I am also tempted to just caveat emptor, but perhaps with some sanity checking on expected results.
However this doesn't dissolve any problems that could occur with dependencies using instanceOf, and it does leave us open to dangerous issues where there may be [invalid] inter op of user land and bitcoinjs-lib depedencies (with different versions).

[coveralls]

Coverage remained the same when pulling 7cd189e on dcousens:expose into 09455a6 on bitcoinjs:master.

[dcousens]

Wrapping BigInteger and ecurve would be painful, and would require us to add testing for those wrappers... not in itself a bad thing, but a lot of work for not much gain.

[dcousens]

Still thinking about this one, seriously considering the more caveat emptor approach for 2.0.0.

Ideally however, I'm hoping to just remove as many non POD types from the API as possible, without any compromise in performance capabilities.

[glorat]

For ongoing food for thought...
http://weblog.jamisbuck.org/2008/11/9/legos-play-doh-and-programming

I still remain firmly on the fence :)

[dcousens]

This problem isn't solved, but this PR isn't the solution. Will close.