Idiomatic API

[mappum]

Bitcore uses an API that Node.js developers will find strange. Parts like the odd pseudo-class system add complexity to learning how to use the library, and some API choices just don't make sense (e.g. using Error throwing in place of returning boolean values, as seen in Address::validate).

Obviously, this is a subjective issue and you may be satisfied with the current design, but I would strongly urge to match idiomatic Node.js API standards.

I will start refactoring on my fork, and you can choose whether or not to accept the pull request.

[jrpereira]

Couldn't agree more, and your post reflects precisely the discussions we've been having internally on our own project, where we're considering using bitcore. We've been hesitant to get involved largely due to many of these choices that make the codebase seem alien and convoluted.

The pseudo-class system is one of those sore thumbs, particularly with ES6 around the corner, which is already supported by tools like browserify (compiling down to ES5). I mean, if syntactic support for classes is here, it makes little sense to start a project that intends to be such a stepping stone and not use it.

The issue with throwing errors is of a different nature. It reflects poor separation of concerns such as logic and actual usage. Using your example, validation is a logical operation, it should not imply an error, seeing as that is an interpretation of a logical operation, it depends on context. For instance, if you're writing a task that attempts that picks invalid addresses in an array, you'd typically use validate in a map or a filter. If validate is throwing an error, you'd have to write a wrapper function, which is unnecessary. Keeping core functions simple and adding context on the caller is much more useful for a core library.

I for one am excited to see movement in this direction. If there's support for these choices and plans to integrate this stuff into master, it'd mean a lot, and we'd be happy to jump on the bandwagon and contribute.

[gasteve]

Classes in ES6 make the same mistakes that every language going back to Smalltalk made. And "Classtool" is really a misnomer (they aren't really classes in the traditional sense), what it allows is a body of related behavior to be instantiated multiple times with different bindings (including different inheritance chains). This is a more elegant solution to the split hierarchy problem (which classes in ES6 will suffer from) than mixins or multiple inheritance. Controlling bindings also allows the code to be easily tested using stubs or mocking.

The only practical difference it makes in coding style is putting the constructor and prototype methods in an outer closure.

[gasteve]

(note: I never liked the term "class" …and if there is a way that we an do "require('bitcore/Address');" rather than "require('bitcore/Address').class();" while still retaining the ability to construct multiple "Address"es bound to the environment in different ways when needed, that would be great

[gasteve]

(btw, I agree with the point about validate() throwing an exception)

[maraoz]

We should tag the current version (0.1.5) and make this changes for next version (0.1.6).

I totally agree about the Address validation problem. Regarding classtool, it's really useful in some cases (especially for testing), so I wouldn't remove it. What I suggest is make it that require('bitcore/Address') returns the class, and require('bitcore/Address').factory returns the class factory (what it's currently returning)

[jrpereira]

@gasteve, I know what you mean. ;) That said, ES6 classes have been debated ad-nauseum elsewhere, so whether we agree or not is a bit beyond the point at this stage.

That said I've been using ES6 (NodeJS harmony) for some time (yield rocks), and it does make the code tidier, easier to understand and harder to break (stop messing with Array.prototype!). Particularly good for when you have teams with multiple languages, the code looks familiar. And in production environment and people with really different backgrounds, that's a plus.

Anyway, like it or not, ES6 is where JS is heading, it will become 1st language of an entire generation of programmers. Given that, we (imho) should at least try and make things look easy to understand and familiar.

I really like the proposal from @maraoz, just make it return the class and have .factory.

[gasteve]

Just because ES6 has classes doesn't mean you have to use them. And until we have 2 or 3 generations of the major browsers that support ES6, we need to stick with ES5. Btw, yield is a really bad idea, but that's a whole 'nuther topic (I used to work on virtual machines and language design professionally and have spent a lot of time (probably too much time) thinking about these things). Btw, the inspiration for classtool came from a pattern found in Newspeak by Gilad Bracha if you're interested: http://newspeaklanguage.org

[ryanxcharles]

It seems we're all in agreement on these two things:

1) Requiring a new class should not rely on .class(), which will cause countless headaches for new developers. We need to change either bitcore and/or classtool to make this happen, and we simultaneously need to retain the existing classtool features, possibly by using .factory()

2) Throwing an error for validating an address is confusing and cumbersome. This and other similar cases should be updated to return "false" or something instead of throwing an error.

Let's follow @maraoz's suggestion and tag the current version 0.1.5, then make these backwards-incompatible changes on 0.1.6. Agreed?

[gasteve]

I like the concept, but as usual, the devil is in the details…I wonder if we could do something like "require.factory('bitcore/Address')" if you wanted to load Address and get just the factory without constructing the default class instance.

Edit: maybe we call it "factory" instead of "classtool" …it would be good to see this mocked up to get a feel for it

Another Edit: also, anywhere we can minimize having to patch module would be good too

[mappum]

The sane, idiomatic way would be:

var Address = require('bitcore').Address;
var addr = new Address('x');

[ryanxcharles]

We already have the bitcore object for the browser code. We could make require('bitcore').Address return the Address class by default. If you want the factory, you can do requrie('bitcore/Address').whatever().

[maraoz]

@mappum That's something I've been working on for the browser version of bitcore. Look at https://github.com/bitpay/bitcore/blob/master/bitcore.js and how it's used in tests: https://github.com/bitpay/bitcore/blob/master/test/test.Connection.js

I agree that'd be a good way to access the API

[gasteve]

Is there a way to support that without loading in the entire library?

[ryanxcharles]

Yeah, the bitcore object is compiled. bitcore.Address can be undefined if you didn't compile it in.

[gasteve]

or maybe "require('bitcore');" loads everything, but you still have the option of "require('bitcore/Address');" if you only need Address validation functionality?

[mappum]

@gasteve This is standard usage of Node. It doesn't make a difference that you load the whole module because if it has already been loaded, it is cached in the Node process and functionally isn't any different than loading a specific part of the module.

[gasteve]

That's not what I mean…let's say there are 1000 files in bitcore, but you only need Address functionality in your app … "require('bitcore').Address;" would load all 1000 class files (slowing down loading and bloating memory usage). "require('bitcore/Address');" would only load Address, not the entire bitcore library.

[mappum]

I think you will find that even with 1000 files, loading the module is extremely fast and consumes an extremely small amount of memory. If it ever became a problem, you would probably want to pull out submodules that are commonly used standalone into their own modules, e.g. require('bitcore-addresses') might include Address, and relevant helper functions.

[jrpereira]

@mappum while that's true for Node, it's relevant for the browser. It's also very good practice to include just what you need. Even PHP went in that direction with namespaces and use instructions. For one, it allows you to more easily guarantee code coverage and avoid side-effects from other code.

Anyway, imho, this would be the "sane" way:

var Address = require('bitcore/Address');
var AdressFactory = Adress.factory;

// and then, which of these would make sense?
var addr1 = new Adress();
var addr2 = AddressFactory.create();

[gasteve]

Address.factory would be for creating a new class instance, not address instances.

var AddressFactory = require('bitcore/Address').factory; var Address = AddressFactory({Base58: {…some Base58 substitute}}); var addr = new Address();

[jrpereira]

@maraoz, I can see how that's useful, to have "container" object for everything Bitcore-related. However, while this can be a convenience, it's the kind of code that belongs at the application level.

For instance, if you're simply doing a small validator of addresses, it makes no sense to include everything - you run your own require calls. Or for instance, if you're doing something that doesn't generate keys, you don't include private key stuff.

That said, the existence of bitcore.js doesn't preclude one from including just the needed bits, it's just a convenience. As long as each individual module does the require() itself rather than relying on a global bitcore object.

[ryanxcharles]

IMO, for node, require('bitcore') should include everything, say require('bitcore').Address. For browser, require('bitcore') should include only the desired classes - if Address was not compiled in, require('bitcore').Address should be undefined.

[jrpereira]

@gasteve, that looks sweet.

How about making it so the default class instance is the exports, and also contain a .factory property? That way you could cover both scenarios:

// Address is the default class instance
var Address = require('bitcore/Address'); // this returns a call to factory()

// Get the factory
var AddressFactory = Address.factory;

// A different class instance
var AddressWithBase58Substitute = AddressFactory({Base58: {…some Base58 substitute}});

var addr1 = new Address();
var addr2 = new AddressWithBase58Substitute();

Granted, this is just a convenience, I'd be happy with what you suggested.

[maraoz]

My vote is for having both options. Most users will want the ease of doing:

var bitcore = require('bitcore');
var Address = bitcore.Address;

For more advanced use cases (performance, or customizing dependencies) you can still do:

var Address = require('bitcore/Address').factory.createClass({base58: ...});

On Thu, Feb 20, 2014 at 1:37 PM, Ryan X. Charles notifications@github.comwrote:

IMO, for node, require('bitcore') should include everything, say require('bitcore').Address. For browser, require('bitcore') should include only the desired classes - if Address was not compiled in, require('bitcore').Address should be undefined.

Reply to this email directly or view it on GitHubhttps://github.com/bitpay/bitcore/issues/84#issuecomment-35640942 .

[jrpereira]

@ryanxcharles I'd say that for node, you don't want to include everything. For instance, consider a small app that only generates vanity addresses. It wouldn't make little sense to make it depend on everything else, right? Unwanted/unverified code bundled by default, that can only be a source of problems & security issues.

Thoughts?

PS: thanks for putting up with all these comments, I know I'm a bit of an outsider, really appreciated.

[ryanxcharles]

I like @maraoz 's suggestion except that, suppose you start using:

var bitcore = require('bitcore') var addr = new bitcore.Address();

...because it's easy. Then later on you want to optimize, so you convert to:

var Address = require('bitcore/Address') var addr = new Address();

... but now you don't have the bitcore object anymore. So you have to update very instance of bitcore.Address to Address.

[gasteve]

@ryanxcharles I wouldn't write code like "new bitcore.Address();" to begin with…to me that just looks ugly, I would instead do:

var Address = require('bitcore').Address; var addr = new Address();

and if you later want to optimize (to not load all of bitcore):

var Address = require('bitcore/Address'); var addr = new Address();

…since you aren't using "bitcore.Address" anywhere, it's only a one line change

[maraoz]

+1! If everyone is happy with having both options I can work on this changes

[ryanxcharles]

:D

[gasteve]

@maraoz yes …one request though…come up with a way of loading that doesn't create the default class instance …and how about the rename from "classtool" to "factory"?

[maraoz]

@gasteve OK, I'll look into that. I like the name factory too :)

[jrpereira]

@maraoz, +2 from my end (proxy vote @fixe). ;)

[maraoz]

I'm starting with this

[maraoz]

https://github.com/maraoz/factory/pull/1

I created a preliminary pull request with these changes. I only tested the changes I made, but in the future I'll add tests for previous classtools functionality, to improve the module.

Can you guys check out if agree with the changes, so I merge them and then work on changing bitcore to use factory instead of classtools?

[gasteve]

two things I don't like …it's still using the term "class" and if you do "require(…).factory;" it will create the default class (this will be problematic for unit tests)...maybe if you want the factory, we could figure out a way to access it with "factory('path/to/File');", which would temporarily set some global or something in the code loading machinery that would disable the creation of the default class and just return you the factory.

[jtremback]

Just wanted to weigh in and support the original post in this thread... I'm glad that this is being moved to a more conventional node.js API. Thanks for all the hard work, I'm really excited.

[ryanxcharles]

I have a proposal for removing classtool https://github.com/bitpay/bitcore/pull/114

[matiu]

Soop adoption: https://github.com/bitpay/bitcore/pull/113 https://github.com/bitpay/bitcore/pull/120 Require on use: https://github.com/bitpay/bitcore/pull/129 Idiomatic address use: https://github.com/bitpay/bitcore/pull/133

seem to resolve all the issues discussed here.