Types

[dcousens]

Adds a basic type checking module to enforce types in a coherent way.
I opted not to use a external module for this because I didn't find one that was similar enough to what I wanted, without adding boiler plate code everywhere.

Open to that idea though if it exists already, as I'm not a huge fan of having this included.

edit: Note, this doesn't add or remove any type checking that wasn't already being done with assert.

[dcousens]

As suggested by @weilu over IRC, probably better to merge this from 2.0.0 now and have #287 just change how custom types are dealt with in one place.

[jprichardson]

I don't think this is a good idea. I think the spirit behind it is noble, but it's not idiomatic JavaScript and feels foreign. I understand the concern... we're dealing with financial applications here and as such, we don't want to cause loss of funds. But there's got to be a better way...

I'm half serious on this proposal... a port to TypeScript?

[dcousens]

I'm actually really keen on the idea of a port to TypeScript, but, for now, this just moves all the type checking code into one place.
It doesn't add or remove any type checking that wasn't already being done.

[dcousens]

I just know how often a burn in this department can be fatal, especially when dealing with Transaction and ecdsa. I wanted to make it consistent, and eventually easier to put under test.

[jprichardson]

Ya, maybe it's a good enough compromise for now? We tackle the TypeScript port later?

[dcousens]

That was my thought also.
I agree it sucks how this is not idiomatic, but, the reality is, that right now, it's just reckless not to.

[weilu]

Does the performance gain justify the readability tradeoff here?

[dcousens]

What readability loss is there? Its quite straight forward?
On Oct 7, 2014 4:19 PM, "Wei Lu" notifications@github.com wrote:

In src/types.js:

@@ -0,0 +1,38 @@
+module.exports = function enforce(type, value) {

• switch (type) {
• // http://jsperf.com/array-typecheck-2
• case 'Array': {

•  if (value != null && value.constructor === Array) return
  

Does the performance gain justify the readability tradeoff here?

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoinjs/bitcoinjs-lib/pull/288/files#r18500957.

[weilu]

not as compared to Array.isArray

[dcousens]

I think the readability trade off isn't an issue as this is encapsulated and (ideally) easily tested code.
It is also very verbose in what is being done anyway, so again, no real concern.

I feel where possible, we should err on using the more performant solution in this case as we don't know how it might affect users?

All in all, there are way bigger fish to fry, so if you feel its important to use the alternative, that's fine.
I'd rather spend time swapping out our BigInteger library before we really try to balance up the pro's and con's of a null+constructor check vs Array.isArray performance.

[weilu]

I'd say hold off performance optimization until there's concrete evidence that this is indeed a performance bottleneck. As browsers/v8 are constantly optimizing, this code could get in the way of performance without us knowing.

[coveralls]

Coverage increased (+0.03%) when pulling 0c380a0 on dcousens:types into 166053a on bitcoinjs:master.

[dcousens]

As per discussion, now uses the idiomatic checks, if they're an issue performance wise (ha), we can easily change them over.

[coveralls]

Coverage increased (+0.03%) when pulling 01a96e8 on dcousens:types into 166053a on bitcoinjs:master.

[weilu]

Now that we use the idiomatic versions for all the checks, do we still need the links to perf?

[dcousens]

@weilu you mentioned in #287 that there was minification issues. Are they an issue here?

[coveralls]

Coverage increased (+0.03%) when pulling 967e724 on dcousens:types into 166053a on bitcoinjs:master.

[weilu]

I don't think so. Where the code for this changeset stands it doesn't use constructor at all, so I believe it should be unaffected by minification.

[weilu]

Verified by browserify test/*.js | node_modules/uglify-js/bin/uglifyjs > test.js then running testling in chrome and safari with the uglified test script