Skip to content
This repository has been archived by the owner on Jan 20, 2020. It is now read-only.

Modernize codebase #40

Closed
wants to merge 1 commit into from
Closed

Modernize codebase #40

wants to merge 1 commit into from

Conversation

fb55
Copy link
Contributor

@fb55 fb55 commented Apr 3, 2017

lib/request_signer was already using a couple of ES6 features, so this shouldn’t break anything.

Switches codebase to classes and arrow functions, replaces lodash with ES5 array extras and normalizes whitespace to four spaces.

@awitherow
Copy link
Contributor

awitherow commented Apr 6, 2017

Currently the test failing is

/home/ubuntu/gdax-node/lib/clients/public.js:58
    get(...args) {
        ^^^

due to unsupported rest parameters.

Support for them is shown here: http://node.green/#ES2015-syntax-rest-parameters

They are flagged as warnings below node v6.4.0.

There is a PR about setting the node engines, #32 but I would like to be a little more 'edgy' with this so we can have a hawt codebase here.

@awitherow
Copy link
Contributor

I pulled this branch and set the node engine to v7.8.0 and all the tests are passing!

The OrderBookSync ones looked a little bit iffy, they took a while to complete.

   OrderbookSync public client should stream trades: 2784ms
   OrderbookSync public client should stream trades with function: 3365ms

@awitherow
Copy link
Contributor

ping @fb55

@awitherow
Copy link
Contributor

ping @mihar @fb55 ? anyone alive?

@awitherow
Copy link
Contributor

@fb55 I would really like to work on getting this merged and working, is there something we can do to get this happening or can the community here get an update at least of what is going on? I have pinged you guys multiple times over the last 2 months... Can we be a little more professional about this?

@fb55
Copy link
Contributor Author

fb55 commented Jun 3, 2017

@awitherow I didn't have a chance to investigate the performance regressions at the time, which is why I didn't pursue it any further. I would love it if someone could take this over.

@nikulis
Copy link
Contributor

nikulis commented Jun 6, 2017

In looking at updating tests for my own refactoring, I found that the current tests for PublicClient#getProductTradeStream were actually going to the network / live GDAX API instead of mocking the requests through nock. The variability of the network and live API timing is likely what lead to the difference in execution time.

I've opened a new PR #55 that has a local copy of the requests for nock to use (I used nock.recorder.rec() and friends to record the live API calls and save them to a nock config json that could by loaded in tests.)

@awitherow
Copy link
Contributor

@fb55 thank you for the response!

Do you guys do any standardized performance testing for gdax-node already, or would we have to create a standard now?

@fb55
Copy link
Contributor Author

fb55 commented Jun 6, 2017

@nikulis That's a great find, thanks for looking into it!

@awitherow We don't have anything like that for gdax-node right now. The existing tests already help to spot large changes, having some proper benchmarks would be great though.

@awitherow
Copy link
Contributor

This would be a learning exercise in terms of how to do benchmark testing & performance stuff... So I am just shooting ideas and needing a little guidance.

Would integration of something like this:

https://benchmarkjs.com

Into the test suite help to track things in the future well?

We could then even write the statistics to json and be able to see the trends in speed of the test suite over time?

Is there a better way to do this?

@awitherow
Copy link
Contributor

Also, what is to be done about the node version of the project? With the refactorings, a higher default node version is necessary for the code to even properly function. Thoughts on using v8.0.0?

@nikulis nikulis mentioned this pull request Jun 9, 2017
@fb55
Copy link
Contributor Author

fb55 commented Jun 15, 2017

Closing in favor of #60

@fb55 fb55 closed this Jun 15, 2017
@CjS77 CjS77 deleted the felix-es6ify branch January 4, 2018 18:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants