Remove include guards

[levino]

I cannot see why they are necessary. I do not find any global variables that need to be protected from being overwritten.

https://github.com/bitpay/bitcore-lib/blob/master/index.js#L7

[levino]

So this is why it was introduced: bitpay/bitcore#1164

But there should be a better way. I am trying to reproduce the error with this example: https://github.com/Levino/understand-instanceof

Can someone help me reproduce it?

Mentioning @fanatid

[levino]

Okay, now I get it. But then one should remove the "instanceOf" calls instead of introducing include guards.

[levino]

So in principle one just wants a sanity check there. I mean if the check fails one throws an error anyhow. So why not just add a string to the PublicKey instances indicating the classname? Something like this here: https://github.com/Levino/understand-instanceof/tree/solution

Should one not be happy then? I agree that people could pass a very old implementation of a Publickey into a new version of bitcore. But maybe it is better to allow this, than to mess this much with the require part of everything.

Edit: The error will be handled quietly in most cases, if I understand correctly.

[levino]

Another solution could be to make PublicKey (and so on) their own packages and require them with:

var PublicKey = require('bitcore-publickey')

Am I wrong?

[levino]

Mentioning this #4

[fanatid]

If you have correct package.json now (use peerDependencies in modules such as bitcore-mnemonic) or use npm@3 that install modules maximally flat most likely you willn't have problems with version guard. But in general I agree, instanceof check should be replaced.

[levino]

@fanatid Thank you for pointing this out. Very interesting. Here is a read on peerDependencies: https://nodejs.org/en/blog/npm/peer-dependencies/

So I understand that moving this line to "peerDependencies" should solve all problems: https://github.com/bitpay/bitcore-mnemonic/blob/master/package.json#L41

Include guards can stay, even though I would prefer a warning instead of an error.

[fanatid]

Yep, I think that should solve problem.
At least I put bitcore-lib to peerDependencies in coloredcoinjs-lib and use this library in chromanode. All fine, on npm@2 and npm@3

[levino]

Just tested it. Requiring this version of bitcore-message and bitcore 0.13.6 in one application gives a nice error.

npm ERR! Linux 3.19.0-33-generic
npm ERR! argv "/home/levin/.nvm/versions/node/v4.2.2/bin/node" "/home/levin/.nvm/versions/node/v4.2.2/bin/npm" "install"
npm ERR! node v4.2.2
npm ERR! npm  v2.14.7
npm ERR! code EPEERINVALID

npm ERR! peerinvalid The package bitcore-lib@0.13.6 does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer bitcore-message@1.0.2 wants bitcore-lib@^0.13.7

So please @braydonf and team, update the plugins, use peer dependencies and stop using include guards.

[paulkernfeld]

I would really appreciate this, because I'm finding it difficult to develop locally using npm link. This is because npm link may result in multiple copies of bitcore-lib. I understand that this is intended to keep users from getting tricked by instanceof, but this is preventing my project from running in a situation where I'm not even depending on bitcore-lib directly. I think the include guards have the potential to disrupt builds on any project that has bitcore-lib anywhere in the dependency hierarchy.

[levino]

@paulkernfeld I agree totally. It is very un-npm-style to even think of introducing them. Really bad style. Take this fork if you want only warnings and not exceptions https://github.com/satoshipay/bitcore-lib

[paulkernfeld]

@levino thanks! I think your link is broken; isn't that the main bitcore-lib repository?

[paulkernfeld]

I believe that @levino is referring satoshipay/bitcore-lib, which changes this to a warning.

Unfortunately, this doesn't fix my problem, because a dependency that I don't control (webcoin) still depends on the original version of bitcore-lib. :(

To the bitcore team: what would it take to fix this? Would removing the instanceof calls be enough?

[paulkernfeld]

In the mean time, here is a fun and sketchy workaround for anyone who is interested! I probably don't need to say that this is not the ideal solution.

// Workaround for https://github.com/bitpay/bitcore-lib/issues/21
global._bitcore = undefined
var Script = require('bitcore-lib').Script
var Transaction = require('bitcore-lib').Transaction
global._bitcore = undefined

[levino]

@paulkernfeld Yes, sorry, took the wrong one. I corrected the link in my comment above. Deleting the include guards works also. See also here: bitpay/bitcore#1188 (comment)

[wzrdtales]

+1
Totally agree to this, this module is completely broken through this, taken how easy this error will throw when adding just one single dependency.

@braydonf
Can you explain why the bitpay team has decided to use global variables? I also had my experiments with them in modules and threw them out very quickly again, but they are really only useful for final applications, but not for modules.
Or are there any other reasons why this module would want to throw such errors?

[matiu]

Totally agree to this, this module is completely broken through this, taken how easy this error will throw when adding just one single dependency.

no, it is not.

Because Bitcore-lib use is instance of, see for example https://github.com/bitpay/bitcore-lib/blob/master/lib/address.js#L102, it needs to have only one declaration of each class.

If you run into the error, you can fix it doing the following:

1- Check were your ware including bitcore-lib: In the root of you project run:
fing . -type d -name bitcore-lib

2- You must have 2 or more results. In order to fix the error you need to have only one result.

3- Leave only one, and remove the others, but deleting the directly and adding a symlink to the one you left.

That should fix the error. I understand it is a bit frustrating and probably there are better ways to solve this in the code, but this procedure should work on all cases.

[levino]

Because Bitcore-lib use is instance of, see for example https://github.com/bitpay/bitcore-lib/blob/master/lib/address.js#L102, it needs to have only one declaration of each class.

Maybe one should not check for "instance of"? I would say this is very very bad practice. It might be something that works in C++, Go or wherever stuff is type safe. But in JavaScript you just "try your best" with whatever you get. And if some dumb user gives you an object that does not fit the requirements, it is his bad. BOOOM exception. The checks you are mentioning have to ALSO be removed of course.

[braydonf]

Considering that instanceof assertions for PublicKey have found bugs (indutny/elliptic#17) that could lead to the loss of funds, the extra safety is worth it.

[levino]

Aha. This is helpful @braydonf But then is there not a better way? How exactly does the test help?

[braydonf]

Moved to: #67