Promisify #60

nikulis · 2017-06-09T18:17:50Z

Here's my first pass at adding in promise support re: #41.

The main area of interest for this PR is the tests directory (re: discussion on #55), where I've reorganized the test suites and added tests that exemplify uses of both callbacks and promises where applicable.

When I started work on this, I had not yet realized that the blocking performance regressions on #40 were due to how tests were running instead of introduced code complexity, so I did my own port to ES2015/17 (also just to get myself familiar with the codebase). I don't mean to replace the hard work others have done on #40, and would be happy to port my changes over to that branch after the maintainers have decided on Node version compatibility and updated the CI to build accordingly.

- Refactor library codebase to use native ES6 syntax, including classes - Remove dependency on lodash functions - Update dependencies to latest versions - Refactor test code to use `const`, arrow functions where possible - Normalize codebase formatting using prettier

Added tests to ensure that, for methods which support callbacks and return promises, that both the callback and promise give the expected behavior. Reorganized test suite to use `tdd` test ui and use mocha's `spec` reporter for better printout of test hierarchy.

…c.js`. Also move mocks JSON into a new folder

awitherow · 2017-06-10T11:05:43Z

lib/clients/authenticated.js

-      callback = orderID;
-      callback(new Error('must provide an orderID or consider cancelOrders'));
-      return;
+    if(!orderID || typeof orderID == 'function') {


Is the typeof 'function' check necessary here?

I think their original intent was to ensure that the first argument was an orderID instead of a callback. So if orderID were a function, then !orderID === true, so you have to also check to make sure it's not a function.

ahh yeah right, I didn't think about it enough 👍

awitherow · 2017-06-10T11:06:12Z

lib/clients/authenticated.js

-    return prototype.get.call(self, ['accounts'], callback);
-  };
+  getAccounts(callback) {
+    return this.get(['accounts'], callback);


thoughts on using arrow syntax in classes?

How so? If you mean something like

class Whatever { getAccounts: callback => this.get(['accounts'], callback); }

I'm pretty sure that's invalid JS syntax.

class whatever { func = prop => this.otherfunc() }

works?

Not until ES Class Fields are finalized (currently stage 2). Would (/ will?) be nice though.

Jumping between babelified React Native, React and Node can get confusing 😅

I feel your pain!

awitherow · 2017-06-10T11:06:31Z

lib/clients/authenticated.js


-    args = args || {}
-    if (!callback && (typeof args === 'function')) {
+  async cancelAllOrders(args = {}, callback) {


awitherow · 2017-06-10T11:07:44Z

lib/clients/authenticated.js

-          return;
-        }
+        totalDeletedOrders.push(...data);
+      } while (data.length);


elegant while!

awitherow · 2017-06-10T11:08:11Z

lib/clients/authenticated.js

-      callback(new Error('must provide an orderID or consider getOrders'));
-      return;
+  getOrder(orderID, callback) {
+    if (!orderID || typeof orderID == 'function') {


again on the use of the orderID function check.

awitherow · 2017-06-10T11:10:44Z

lib/clients/public.js

-    args = args || {}
-    if (!callback && (typeof args === 'function')) {
+  getProductOrderBook(args = {}, callback) {
+    if (!callback && typeof args === 'function') {


These couple lines of code are duplicated so much in this repo in general, thoughts on what could be done to reduce that in a functional way?

In terms of optimizing syntactic aesthetics, decorators are the first thing I think of,

@optionalArgs someFunction(args, otherParam, callback) { /* ... */ }

But even though decorators are in Stage 2, there isn't great out-of-the-box support for transpiling them quite yet, including in the latest version of babel.

Yeah definitely something for the future 🌈 🦄

awitherow · 2017-06-10T11:11:53Z

lib/clients/public.js

+          // rate-limited, try again
+          setTimeout(() => {
+            fetchTrades.call(this, stream, tradesFrom, tradesTo, shouldStop);
+          }, 900);


why 900? Just curious about the setTimeout.

Not sure, that's the timeout that was used previously.

Hmm interesting. 👍

The rate limit is per second, this skips to the next second. Not sure if this is the ideal solution, we could also just trigger a couple more requests.

awitherow · 2017-06-10T11:14:01Z

lib/clients/websocket.js

-    var self = this;
-    self.emit('open');
+  onOpen() {
+    this.emit('open');


confused at this here... onOpen is called when 'open' is emitted, and then it immediately emits 'open' again?

Hmm, now that you point that out, I'm confused as well. This was the previous behavior. Thoughts @fb55?

fb55

Sorry for the long wait — this is looking really promising (no pun intended). I left some comments, but this should be good to go soon.

How do you feel about integrating #58 (with default whitespace settings, I didn't see that we were using two spaces here)?

edit: Also, you will need to set the minimum node version for circle to pass.

fb55 · 2017-06-15T21:26:28Z

lib/clients/authenticated.js

+      do {
+        // Have to use a special promise to get response as well as data
+        // for later passing to the callback;
+        [response, data] = await new Promise((resolve, reject) => {


I'd prefer to not use async/await in this library for now (node@6 should be the target).

fb55 · 2017-06-15T21:27:52Z

lib/clients/authenticated.js

+          this.delete(['orders'], opts, (err, response, data) => {
+            if (err) reject(err);
+            resolve([response, data]);
+          }).catch(() => {}); // silently ignore, rejection is already handled in callback


Ideally this would be appended every time we pass a callback.

fb55 · 2017-06-15T21:28:44Z

lib/clients/authenticated.js

-    if (!callback) {
-      throw "Must supply a callback"
-    }
+    if (!callback) callback = () => {};


Let's handle this in makeRequestCallback

fb55 · 2017-06-15T21:29:49Z

lib/clients/public.js

      if (!started) {
        started = true;
-        fetchTrades(self, rs, tradesFrom, tradesTo, shouldStop, 0);
+        fetchTrades.bind(this)(rs, tradesFrom, tradesTo, shouldStop, 0);


Use fetchTrades.call here.

fb55 · 2017-06-15T21:31:15Z

lib/clients/public.js

+          // rate-limited, try again
+          setTimeout(() => {
+            fetchTrades.call(this, stream, tradesFrom, tradesTo, shouldStop);
+          }, 900);


The rate limit is per second, this skips to the next second. Not sure if this is the ideal solution, we could also just trigger a couple more requests.

fb55 · 2017-06-15T21:32:11Z

lib/clients/websocket.js

+const EventEmitter = require('events').EventEmitter;
+const Websocket = require('ws');
+const Utils = require('../utilities.js');
+const signRequest = require('../../lib/request_signer').signRequest;


Destructuring for EventEmitter and signRequest — should also be enforced by eslint (prefer-destructuring).

- Add engine `node >= 6.0.0` - Refactor out (async/await) --> es2015 -`extend:eslint-recommended` - Refactor a bit of code to match

…tCallback`

nikulis · 2017-07-04T04:39:33Z

Hmm, CircleCI seems to have independently fallen down on this one ("An error occurred while fetching information about this build from GitHub.").

I spun up my own Circle instance and it seems to be working fine, though. Can anyone with access go in and trigger a re-build?

nikulis · 2017-07-04T04:41:39Z

Side note: It seems that Circle doesn't read node engine settings from package.json, so we have to use a circle.yml config file for now.

Also introduce a few wording, stylistic tweaks. Correct markdown syntax

Also add relevant test case This addresses coinbase#23 and coinbase#66

If a client method is invoked with a provided callback, the ultimately- returned promise will never be fulfilled (neither resolved nor rejected). This prevents potential `UnhandledPromiseRejectionWarning`s, which will cause future versions of Node to terminate.

nikulis · 2017-07-12T17:05:34Z

Just checking in - would anyone have time to review these newer changes?

If all is well, I propose merging this branch and getting a new release out very soon, possibly with some other addressed issues as well. Thinking towards the future, I also still have nikulis#1 open (based on this branch) which implements the more standard two-argument callback API (major breaking change).

There are many other open issues that I would like to work on, but I am trying to limit the scope of this PR itself. If the maintainers are interested in moving forward with this PR (as is my impression), I think it would be valuable to get it through soon; there seem to be many community members submitting PRs based off of the current master which would all need significant refactoring in order to apply here, and I think it would be fairer to them to not have to duplicate so much work going forward. That being said, I'm always happy to help with related tasks too :)

Thanks!

fb55

Sorry that it took this long to do another review. I very much like where this is going, and I agree this should land soon to avoid merge conflicts.

We should revisit the promise interface, plus there is one remaining bug (a missing .bind). Still, this should be good to go very soon.

fb55 · 2017-07-13T06:26:33Z

README.md

+
+  try {
+
+    let products = await publicClient.getProducts();


We should standardise to either use let or const in this file. I'd opt for const.

fb55 · 2017-07-13T06:37:26Z

lib/clients/authenticated.js

-    opts = opts || {};
-    if (!callback && (typeof opts === 'function')) {
+  request(method, uriParts, opts = {}, callback) {
+    if (!callback && typeof opts == 'function') {


=== for typeof checks

fb55 · 2017-07-13T06:42:08Z

lib/clients/public.js

+        callback(err, response, data);
+        return;
+      }
+      if (err) reject(err);


if/else should always have blocks.

fb55 · 2017-07-13T06:48:57Z

lib/clients/public.js

-      callback(err, response, data);
+      if (typeof callback == 'function') {
+        callback(err, response, data);
+        return;


We should still fulfil the promise. What do you think about

Having the signed request simply call the unsigned variant (super.request)

Use superagent's promises directly:

Create the promise, store a reference

If there is a callback, add handling for it in a then callback

Return the original promise (not the one with error handling applied) — this will ensure that errors are always forwarded

fb55 · 2017-07-13T06:50:09Z

lib/clients/public.js

      if (!started) {
        started = true;
-        fetchTrades(self, rs, tradesFrom, tradesTo, shouldStop, 0);
+        fetchTrades.bind(this)(rs, tradesFrom, tradesTo, shouldStop, 0);


Use fetchTrades.call here.

fb55 · 2017-07-13T07:01:22Z

tests/websocket.spec.js

-  test('subscribes to the default product (BTC-USD) if undefined', function(done) {
-    var server = testserver(++port, function() {
-      var websocketClient = new Gdax.WebsocketClient(null, 'ws://localhost:' + port);
+  test('subscribes to the default product (BTC-USD) if undefined', function(


All of these should be switched to arrow functions (eslint can also take care of that — only named functions should still be allowed).

fb55 · 2017-07-13T07:02:06Z

.eslintrc.json

+  "root": true,
+  "extends": "eslint:recommended",  
+  "env": {
+    "commonjs": true,


Pretty sure this is implied by node

fb55 · 2017-07-13T07:02:23Z

.eslintrc.json

+    "ecmaVersion": 2015
+  },
+  "rules": {
+    "constructor-super": "warn", 


All of these should error.

fb55 · 2017-07-13T07:05:00Z

README.md

-var callback = function(err, response, data) {
-  // your code here.
-};
+


Perhaps add some description here: "Promises can be used with callbacks, as well as using async/await in modern environments." (or something similar).

fb55 · 2017-07-13T07:18:24Z

lib/clients/public.js

+      if (err) reject(err);
+      if (data === null)
+        reject(new Error('Response could not be parsed as JSON'));
+      else resolve(data);


I've been looking into replacing parts of this library with swagger-codegen. Their promise interface returns a {data, response} object — I guess we should adopt that as well. While I like the simplicity of this API, there probably are cases where you want the request object (for headers, status codes etc.), plus an object would allow for future extensions.

All eslint rules now error instead of warn - Add eslint rule `curly: all` - require block statements for control structures (`if`/`else`, etc.) - Add eslint rule `eqeqeq` - only strict equality operators - Add eslint rule `func-names: always` - no anonymous `function` expressions - Add eslint rule `prefer-arrow-callback: { allow-named-functions: true }` Add `eslint` to list of `lint-staged` actions to be enforced before commits

Remove duplicate request logic from `AuthenticatedClient`; delegate to `super` request method

Add some more descriptive text Use `const` over `let` in code examples

Refactor to use promise interface on public/authed clients' `.getProductOrderBook` Update test mock ws server to respond with expected format (with `product_id` data) Remove unnecessary call to `Util.determineProductIds`

fb55 · 2017-07-17T17:53:43Z

lib/clients/public.js

-    return prototype.get.call(self, ['products'], callback);
-  };
+    if (callback) {
+      p.catch(() => {});


This is definitely sensible behaviour, although I'd simply add listeners to the promise in case there is a callback. Still return the original promise (not the one with the callbacks applied) so that nothing needs to be returned from the callback.

We could also get rid of makeRequestCallback in favour of superagent promises — how do you feel about that?

fb55 · 2017-07-17T17:57:39Z

One comment, but that can be iterated upon once this is merged. There is a merge conflict with the package.json file — once that's resolved I'm happy to merge. Thanks a lot for all the work, this is amazing.

fb55 · 2017-07-26T13:33:12Z

Finally got around to merging this. Thanks a lot @awitherow and especially @nikulis! I'd like to land the superagent switch before publishing it — @nikulis do you want to open a PR with the code you already have?

awitherow · 2017-07-27T09:14:07Z

Thanks to @nikulis I was just giving some insight! He did the 💪. 👏

nikulis · 2017-07-29T22:43:58Z

I was holding off on superagent for this one because those using v0.4.x might be relying upon the currently-exposed response object (2nd callback parameter) that the request library returns. I've personally never used this in my own projects, so I'd have to look into whether the response object could be easily reconstructed using the information that superagent returns. If so, I can

I did switch to superagent with the new two-parameter callback API in nikulis#1. That branch is a little stale atm, so I'm about to work on getting it up-to-date before I open a new PR in this repo. (Just got back online after a few days off 😅 )

And yes, thank you to all of you, too, for providing all the wonderful feedback and critiques!

nikulis added 3 commits June 6, 2017 11:48

Rename test files to reflect their respective src filenames + `.spe…

8eabff7

…c.js`. Also move mocks JSON into a new folder

awitherow reviewed Jun 10, 2017

View reviewed changes

nikulis mentioned this pull request Jun 10, 2017

API Rate Limiting #61

Closed

fb55 reviewed Jun 15, 2017

View reviewed changes

This was referenced Jun 15, 2017

getProductTrades swallows errors #63

Closed

Modernize codebase #40

Closed

nikulis added 3 commits June 16, 2017 13:24

Update engine, eslint settings

edabc12

- Add engine `node >= 6.0.0` - Refactor out (async/await) --> es2015 -`extend:eslint-recommended` - Refactor a bit of code to match

Check callback function before calling it in `PublicClient.makeReques…

4cca17d

…tCallback`

Add prettier config

9c0f5a2

nikulis mentioned this pull request Jun 20, 2017

Project roadmap? #56

Closed

Specify node version in circle.yml for CI testing

51e4a62

nikulis added 3 commits July 4, 2017 02:36

Update README with documentation for promises

d1489a7

Also introduce a few wording, stylistic tweaks. Correct markdown syntax

Update OrderbookSync to emit errors instead of throw

e6a5ae7

Also add relevant test case This addresses coinbase#23 and coinbase#66

fb55 suggested changes Jul 13, 2017

View reviewed changes

nikulis added 9 commits July 13, 2017 21:43

Fix eslint errors in lib

08dc887

Fixed eslint errors in tests

1ace30a

Use call instead of bind for binding this

60e9eec

Use proper Error objects when emitting socket errors

bf46f16

Update API methods to return undefined when given callback

877d2d4

Remove duplicate request logic from `AuthenticatedClient`; delegate to `super` request method

Update wording in README

c2f6b0a

Add some more descriptive text Use `const` over `let` in code examples

Ensure node v6 compatibility for test suite

4ea2aa5

Bugfix: incorrect this binding OrderBookSync internal callback

2943a5a

Refactor to use promise interface on public/authed clients' `.getProductOrderBook` Update test mock ws server to respond with expected format (with `product_id` data) Remove unnecessary call to `Util.determineProductIds`

fb55 reviewed Jul 17, 2017

View reviewed changes

fb55 approved these changes Jul 17, 2017

View reviewed changes

fb55 closed this in a146d93 Jul 26, 2017

This was referenced Jul 26, 2017

integrate prettier for *.js code standardization #58

Closed

update orderbook_sync #66

Closed

Add nock requests for publicClient.getProductTradeStream() tests #55

Closed

Promisify #60

Promisify #60

Conversation

nikulis commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awitherow Jun 11, 2017 • edited

Choose a reason for hiding this comment

nikulis Jun 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fb55 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fb55 Jul 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikulis commented Jul 4, 2017

nikulis commented Jul 4, 2017

nikulis commented Jul 12, 2017

fb55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fb55 Jul 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fb55 commented Jul 17, 2017

fb55 commented Jul 26, 2017

awitherow commented Jul 27, 2017

nikulis commented Jul 29, 2017

awitherow Jun 11, 2017 •

edited

nikulis Jun 11, 2017 •

edited

fb55 left a comment •

edited

fb55 Jul 13, 2017 •

edited

fb55 Jul 13, 2017 •

edited