Refactorized WebSocket API.

[yagop]

I've rewritten everything on ws.js, now events are provided for every message.

Events trade, orderbook and ticker receives 2 arguments as callback, the first is the pair name and second the data. See the example.

Included documentation (on DOCS.md) can be generated with npm run docs. API Rest doc should be written too.

[yagop]

Up

[joshuarossi]

this looks pretty awesome, i am going to play with it now. do you want to chat about it a little?

[yagop]

Of course, tell me your thoughts.

BTW, I took freedom for deleting some unnecessary files like npm-debug.log (added to gitignore too) and old_index.js (it isn't used anywhere). Maybe I should have done another PR for that.

source/images should be deleted and ignored too, it's code coverage which is generated.
Have you thought about using coveralls for showing them?

[joshuarossi]

I was curious about the new documentation (npm run docs), I was using slate for the docs with github pages. I would like to know more about coveralls. One design decision i was contemplating was to accept an object as the argument instead of individual arguments...

[joshuarossi]

I know when I am crafting an order it is more comfortable to be able to work on the order object and play with its attributes and then just pass that one argument...

I really like what you have done, I was wondering if you have looked into the classes in ECMA 2015 and if they could be better than the prototype method of creating objects...

In general, you seem to be a much better js developer than myself, and I love what you have done

[joshuarossi]

i believe source/images was being used to create the logo in the top left corner

[joshuarossi]

anyway, if you are available to chat via Skype, this is exactly the type of interaction we hope to have regarding our libraries, really happy to see your interest, and I think we are going to have rewards for outstanding members of the community.

I'd love to pick your brain on the API and what you think and how it can be improved, and how the library should function as a means to interacting with it.

I was actually discussing individual event emitters because it seemed such a cleaner way of handling different situations, rather than having a massive "onmessage" we can have onticker, onbook, etc

Anyway, the designer of the WS API, loves your suggestions and we are going to merge them as soon as we can do some testing

[yagop]

I was curious about the new documentation (npm run docs), I was using slate for the docs with github pages.

I use JSDoc to document JS libs, its well spread and many modules uses it. JSDoc generates HTML but jsdoc-to-markdown generates markdown that can be shown in GitHub very easily. Slate is really cool for documenting API and writing examples but I don’t find usable for documenting a module.

One design decision i was contemplating was to accept an object as the argument instead of individual arguments... I know when I am crafting an order it is more comfortable to be able to work on the order object and play with its attributes and then just pass that one argument...

I think you are pointing something like this, right?

/**
* @param  {string} pair      BTCUSD, LTCUSD or LTCBTC. Mandatory param.
* @param  {object} [options] Object of additional options
* @param  {string} [options.precision] Level of price aggregation P0 (default), P1, P2, P3.
* @param  {string} [options.length]    Number of price points. 25 (default) or 100.
*/
BitfinexWS.prototype.subscribeOrderBook = function (pair, options) { ...

I prefer it because new parameters can be sent to the API without having to write new code. Just depends if you plan to add new parameters to the API. Another obvious benefit is the number of arguments are less but having only 3 is not too much.

I really like what you have done, I was wondering if you have looked into the classes in ECMA 2015 and if they could be better than the prototype method of creating objects...

Thank you! 😊. Yes, I've played with ES2015 classes but they are only syntax sugar influenced by CoffeeScript. Well I like that sugar but I'm not using it in my public modules. The reason is many developers still uses NodeJS v0.10.x and they wont be able to use the module.

In general, you seem to be a much better js developer than myself, and I love what you have done.

Thanks again, I've been working hard with JS for long 😸

[yagop]

i believe source/images was being used to create the logo in the top left corner

The Travis badge? Travis try to pass tests and won't use the code coverage (at last that is what I know).

I would like to know more about coveralls.

Coveralls is a free service to show tests coverage. A coverage badge is provided for each project by Coveralls so you can show that badge in the project like the Travis one.

The code coverage can be generated by Travis and sent to Coveralls by scripting in package.json, for example if using istanbul.

// Taken from https://github.com/nickmerwin/node-coveralls
"scripts": {
    "test": "mocha test/index.js",
    "test-cov": "istanbul cover ./node_modules/mocha/bin/_mocha --report lcovonly -- -R spec && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage",
  },

There are other services for showing code coverage like Codecov but I haven't tried it.

[yagop]

anyway, if you are available to chat via Skype, this is exactly the type of interaction we hope to have regarding our libraries, really happy to see your interest, and I think we are going to have rewards for outstanding members of the community.

Sure, you can contact me through Skype as yago.ftw@outlook.es or Google Hangouts as yago.ftw@gmail.com . hahahah that sounds awesome.

I'd love to pick your brain on the API and what you think and how it can be improved, and how the library should function as a means to interacting with it.

You mean this? or the server side API implementation?

[joshuarossi]

So, I merged in your changes, it looks really good. It automatically passed all the tests as well, so that is great.

One thing I was curious about, in the past, I had made the pair argument optional, because the vast majority of people care about BTCUSD and that is a sensible default, what do you think?

[joshuarossi]

also published to npm and moved version up to 0.2.0

[joshuarossi]

travis does run the code coverage i have, you can see it at the bottom of the test

=============================== Coverage summary ===============================
Statements : 79.41% ( 270/340 )
Branches : 63.54% ( 61/96 )
Functions : 82.81% ( 53/64 )
Lines : 79.65% ( 270/339 )

https://travis-ci.org/bitfinexcom/bitfinex-api-node

[joshuarossi]

one of the main issues with code coverage is how do you test stuff that requires money?

[joshuarossi]

also you can see the docs have been updated using the docs generated via jsdocs (i just added them into the slate includes folder (might look into changing this in the future, but i would like to include example code of how to use the library)

[joshuarossi]

the docs are here http://bitfinexcom.github.io/bitfinex-api-node/

[joshuarossi]

and coverage report here
http://bitfinexcom.github.io/bitfinex-api-node/images/coverage/lcov-report/index.html

[joshuarossi]

it might be out of date now, not sure

[yagop]

One thing I was curious about, in the past, I had made the pair argument optional, because the vast majority of people care about BTCUSD and that is a sensible default, what do you think?

I wrote subscribeOrderBook and subscribeTrades to take pair as optional because is how it was done before. I would prefer as a mandatory field but its OK how its done I think.

one of the main issues with code coverage is how do you test stuff that requires money?

No, Travis.org pass the tests free and can send the code coverage free too. Everything is free.

travis does run the code coverage i have, you can see it at the bottom of the test

I know. I wouldn’t generate the code coverage by test script but in test-cov which would be executed if tests are passed by Travis. The test-cov would send them to Coveralls too.

[joshuarossi]

ah, not the coverage costs money but testing an order or testing receiving an execution message, those events won't happen unless a financial transaction happens...so it is hard to have a built in test (because it would require a real account with real money to place the orders, etc)

[joshuarossi]

and yes, i am interested in adding coveralls