node-gitlab 2.0 refactor

[dave-irvine]

We're considering the following for a 2.0 refactor that will break backwards compatibility:

• Deep refactoring of library structure to use dependency injection.
• All library calls can return Promises, or take a callback argument.
• In the case of a callback argument, the callback should follow the node.js convention of function (err, result).
• All functions will take options parameter, rather than mixing up what parameters they take. See Fix #70 #72 for more details.
• On successful API calls, we will parse the JSON response and return class Instances to the Promise or callback. See Should we parse the output of the API? #65 for a more detailed explanation.

We'd like to get community thoughts on this refactor. We're considering maintaining a 1.x branch for a while to address any outstanding critical issues.

[moul]

Also a big question is about staying with CoffeeScript versus going to plain Javascript

I personally like enforced scoping CoffeeScript introduce, but we saw multiple times developers complaining about the language. As the project is growing, I prefer to use the most accessible language so we can get more developers to contribute.

[mdsb100]

How about return a backbone model?

[dave-irvine]

@moul re: CoffeeScript, heh, this project is the reason I learned CoffeeScript and now prefer it over JS :D

[dave-irvine]

@mdsb100 I don't think using Backbone is a fit for what we are doing:

Philosophically, Backbone is an attempt to discover the minimal set of data-structuring (models and collections) and user interface (views and URLs) primitives that are generally useful when building web applications with JavaScript.

We're building a library interface to the API, not a web application. GitLab itself may want to use Backbone in their interface, but it doesn't apply to our use case.

[mdsb100]

Titanium use backbone model. Please check http://docs.appcelerator.com/titanium/latest/#!/guide/Alloy_Collection_and_Model_Objects
I suggest we can reference concept of backbone model, it is easy for using.

[dave-irvine]

Again I think this is because that is used for building applications, with user interaction, rather than an API wrapper. This is similar to having us put a CLI in our library, you should use our library to create a CLI (which you have done). You could wrap the objects we return in Backbone models.

[moul]

We can try to write or find an already existing Blueprint of the Gitlab API (https://apiblueprint.org), then we can generate a tree definition of the API calls, parameters, return values, errors
It may replace all repetitive code

@dave-irvine yes, I also prefer CoffeeScript but I still can do vanilla JavaScript, and we can replace the coffee compiler in the Makefile by some JavaScript linters to keep a constant code quality

[mdsb100]

OK, I got it. The node-gitlab just be a backbone.sync.

[moul]

Maybe the Backbone object can be one of many interfaces.
We can add a method to configure a data processor, then we can write one that returns Backbone objects as we can write another for Angular's RestAngular, etc

But I think we need to keep the original object as minimalist as possible

[mdsb100]

I think #65 is not suitable. If we try to do this, then we must create some models to store or control data.

[dave-irvine]

@moul I think I like the idea of APIBlueprint, but doesnt GitLab themselves need to maintain that?

[moul]

@dave-irvine yes I think they need to, if they don't already have it, we can create a pull request on their repository with the base file

[RobinQu]

Firstly, we should use node callback conventions. It's deadly important.

[dave-irvine]

@RobinQu As long as we expect callbacks to be function(err, result, ...), does that work for you?

[RobinQu]

@dave-irvine That would be fine.

[JogoShugh]

I definitely like the idea of supporting Promises. I've worked with both Bluebird and request-promise before. PromisifyAll from Bluebird has worked really well for us.

I installed node-gitlab, then I modified a few of the functions to have function(err, data), then used babel to try it all out using async/await sugar over promises. It worked great! Here's the sample:

https://gist.github.com/JogoShugh/ea69d69c51e78fa42eba#file-asyncawait-js

I saw someone had submitted a pull-request with Promisify stuff in it, but I guess it's outdated now?

Here is another example of where we use it:

https://github.com/openAgile/eventstore-client/blob/promisify-all/lib/projection.js#L72

So long as the callbacks follow the node callback conventions, it's really that simple!

[sivakumar-kailasam]

👍 for Promises.

Can understand why this project uses coffeescript since this started in 2012, but moving away from coffeescript to es6(with babel) would definitely attract more developers to contribute to this project. It would allow a module level import like lodash.

Options object as the argument definitely feels like a natural choice over here.

[garrettjoecox]

+1 for node convention callbacks. Also not sure who's idea it was to return true if something already exists but an informative error would be a lot nicer.

[hutson]

I would agree that at a minimal following the Node convention would be a tremendous improvement in the consumer experience.

Second, I've also used Bluebird to promisify the GitLab interface, similar to @JogoShugh. I haven't quite made it to transpiling with Babel to get async/await, but I'm excited about the prospect. The example provided by @JogoShugh is extraordinarily concise, and easy to read (no nested callbacks, and no then boilerplate).

As for coffeescript versus JavaScript, personally I'd prefer native JavaScript using ES6 features available in Node LTS versions (or at most, babel). Then again, I'm not the one that's going to have to write, or support, this library, so my preference carries little weight. I will say that adding a feature to node-gitlab did take a little while to get up to speed with CoffeeScript. Though I did like the concise syntax of CoffeeScript over the verbosity of JavaScript. If that can be achieved with ECMA2015, I'd go that direction.

Lastly, I agree with @garrettjoecox. I just spent quite a while trying to figure out why I was getting true for a response from a function in node-gitlab for all cases other than success. I'd prefer either the Node convention of an error object (with meaningful information embedded), or a promise rejection.

[tonyfahrion]

Hi, reading the comments I remembered, that there is a discussion at gitlab about swagger (as far, as I know) something similar to blueprint.

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2397

Just for the record :)

[dickeylth]

One year has passed, is the refactor is still WIP? Looking forward to easier combination with Promise.

[cwtuan]

Promise+1