Bitcoinjs-lib: Prefer properties over getters (discuss)

[dcousens]

Discussion requested.

@junderw @dabura667

We typically sprinkle an unhealthy mix of getNetwork and .network throughout for many different fields.

Should we preference to read-only Object.defineProperty's where possible or maintain get___?

Transaction.prototype.getId could become tx.id ...

Related:
https://github.com/bitcoinjs/bitcoinjs-lib/pull/1025
https://github.com/bitcoinjs/bitcoinjs-lib/pull/1062

[junderw]

I prefer:

1. .network, .getNetwork(), .setNetwork() exist for every attribute
2. for internal attributes, .getNetwork() and .setNetwork() throws (I don't think you can make a read of .network throw in JS... can you?
3. for booleans, getNetwork() has an alias isNetwork() (booleans are usually "readable" etc... so it would be isReadable() which is a lot more.......... I'm sorry, unintentional dad-joke...... readable.

It's very java-esque, but I like it.

[junderw]

We could also disallow touching .network, then we can put type checking using typeforce in the set___() functions.

[junderw]

Object.defineProperty would be fine.

I just think the important part is consistency.

[dcousens]

@junderw we typically don't maintain any of the invariants of our structures if users are modifying them underneath us.
We _very_ rarely provide set__ except in a few rare cases where it does feel clunky.

I just think the important part is consistency.

Agreed.

I don't think you can make a read of .network throw in JS... can you?

Yes?

See https://github.com/bitcoinjs/bip32/commit/a037794e2189e55bd3f6850bfdd397034e9a26f7 for an example of how this might apply to ECPair...

[dcousens]

> let x = {}
undefined
> Object.defineProperty(x, 'y', { get: () => 2 })
{}
> x.y
2
> x.y = 3
TypeError: Cannot set property y of #<Object> which has only a getter

[jprichardson]

With the clean ES6 syntax, modern JavaScript code will move to getters / setters unlike the horrible verbose Java syntax (getX, setX). Let's use them and move on - especially since they've been with us since ES5.

[junderw]

Yes?

So if I do:

let o = {}
// do something to add .network in a way that reading it would throw
let x = o.network
// This throws, while internal functions accessing this.network can read it perfectly fine.

What is that "something"?

you could (I guess) have logic in the getter to check the context in which it is being called?...

I am sorry... let me take a step back; one of the goals is to prevent people from messing with internal attributes and create a "private" variable a la golang / java / etc etc... correct? I'm just wondering if / how we could do that.

[dcousens]

I am sorry... let me take a step back; one of the goals is to prevent people from messing with internal attributes and create a "private" variable a la golang / java / etc etc... correct? I'm just wondering if / how we could do that.

Ah, no. We will just use a closure for this functionality. You can't otherwise prevent reading it.

[rbndg]

I like the ES6 getter syntax, which is also supported in the ES6 class syntax

let obj = {
  get hello() {
    return "world"
  }
}
console.log(obj.hello) // prints world

[dcousens]

@rbndg we are at Node 6 for LTS... is that syntax supported yet?

[rbndg]

@dcousens Yes, it is supported

[dcousens]

Properties it is.

ECPair/HDNode/BIP32 are easy done.

What about Transaction...
Do we replace getId with .id?

[junderw]

for consistency's sake, yes I think we should.

[dcousens]

This has issues with Transaction as pointed out in https://github.com/bitcoinjs/bitcoinjs-lib/pull/1086#issuecomment-390901643 and https://github.com/bitcoinjs/bitcoinjs-lib/commit/d85c4c9b9b21cf523e0df6156417ac87f54df9e8

This needs more discussion

[dcousens]

The problem is the properties doing work merely by observing them, e.g a hashing function for .hash will run many times, even if we only wanted to check the type of that property... (using typeforce).

Perhaps this is a failure of typeforce (compared to say ubiquitous usage of typescript), more than a failure of the property approach though.

[junderw]

tl;dr TypeScript ftw?

That's the vibe I'm getting from this discussion...

sprinkling getters with caches gets a bit messy. (re: my old Transaction refactor)

[dcousens]

@junderw indeed. I think, as long as this library is based in JS and uses typeforce, we can't fully transition this... (except for lazy immutable structures, like ECPair, which survive OK because they are merely lazy, not inherently mutable...)

[junderw]

Closing since we moved to TypeScript...

Still using typeforce, and it's a tad clunky... but it's nice to have runtime type checking... :-/

I've made a few stabs at trying to rewrite typeforce in TypeScript to no avail...