remove check for function named 'BN'

[calvinmetcalf]

fix #133 and thus browserify/browserify-sign#27

[jprichardson]

utACK.

[calvinmetcalf]

ut?

[jprichardson]

ut?

ut => "untested" :) i.e. the code looks good, but I haven't actually checked out the code and ran tests personally. I think I picked up the term from the Bitcoin repo.

[calvinmetcalf]

ah

[indutny]

You know what I learned recently? This thing does not work in IE: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name .

Looks like we may need to fallback to using just Array.isArray().

[axic]

You could still have a constant exported in BN and check against that.

[indutny]

Like some magic value?

[axic]

Yes.

Not entirely sure if it would make sense using a function reference, but it might be handy: num.isBN === BN.isBN

This shouldn't work if num is an instance of a different BN - do we want that to work or fail?

[indutny]

If we wanted to check against just one instance of BN - we would use obj instance of BN, right? 😉

[axic]

Well that seems to have good browser support :)

The question is still there: that will still reject BN instances of different versions. I am pretty sure that in some browserified instances that will happen. I think it is the desired behaviour, but I don't think many libraries are prepared for this.

[axic]

@calvinmetcalf @jprichardson @indutny in the previous in line comment, as a suggestion this came up: num instanceof BN.

Would that work in all cases? It rejects any instances from a different BN version, which I think is fine, but has its own drawbacks - I assume many projects aren't prepared for that either.

[fanatid]

@axis instanceof is not good here
bitpay/bitcore-lib#21

[axic]

@indutny @calvinmetcalf @jprichardson @fanatid ping. A decision and fix could be useful ;)

[jprichardson]

I've reversed my position on this. I think function.constructor.name is a good idea to keep. Uglify allows you to not touch this --keep-fnames and IE users can always shim. The nice thing is that different BN.js dependencies than can be used and we don't get caught with instanceof bugs. My two cents.

[calvinmetcalf]

@jprichardson yeah but most/all people don't use --keep-fnames so it would just end up broken in reality, so I'd be in favor of this

[indutny]

Considering recent stability of bn.js versions, I'm inclined to say that we may want to just do instanceof, and add an assert like this:

assert(other.name === this.constructor.name && Array.isArray(other.words) && !(other instanceof BN), 'Looks like you have a version mismatch');

What do you guys think about this?

[calvinmetcalf]

+1 I like it

On Fri, May 20, 2016 at 4:58 PM Fedor Indutny notifications@github.com
wrote:

Considering recent stability of bn.js versions, I'm inclined to say that
we may want to just do instanceof, and add an assert like this:

assert(other.name === this.constructor.name && Array.isArray(other.words) && !(other instanceof BN), 'Looks like you have a version mismatch');

What do you guys think about this?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#134 (comment)

[axic]

@calvinmetcalf @indutny I think this can be closed then?

[indutny]

I think yes, thank you!