Add pageArgs to the getProductTrades definition #307
Conversation
index.d.ts
Outdated
| @@ -162,8 +162,8 @@ declare module 'gdax' { | |||
| getProductTicker(productID: string, callback: callback<ProductTicker>): void; | |||
| getProductTicker(productID: string, ): Promise<ProductTicker>; | |||
|
|
|||
| getProductTrades(productID: string, callback: callback<any>): void; | |||
| getProductTrades(productID: string, ): Promise<any>; | |||
| getProductTrades(productID: string, pageArgs: PageArgs, callback: callback<any>): void; | |||
There was a problem hiding this comment.
Can this be marked as optional?
There was a problem hiding this comment.
It looks like it needs a second set of defs. I'll get that fixed up
There was a problem hiding this comment.
We need both lines, since one returns a Promise and the other returns void and has a callback arg. All the gdax-node functions take a callback arg and use it if provided. If there is no callback then the functions return a Promise.
This works, however, the optional isn't consistent with the rest of the file. Maybe make options optional for all the other ones that return a Promise.
getProductTrades(productID: string, options: PageArgs, callback: callback<any>): void;
getProductTrades(productID: string, options?: PageArgs): Promise<any>;
There was a problem hiding this comment.
@blair I pushed up a match to how the rest of the file does it. There are others on the PublicClient that are still missing types for the options. If you would like, I can in a different pr go back through and add more types and switch the promises over to use the ? syntax.
There was a problem hiding this comment.
The new commit LGTM. How much work you want to put into updating all the calls up to you, but this looks fine for me.
Fix for issue: coinbase#306
wilwade
force-pushed
the
fix-getProductTrades-ts-def
branch
from
8a046d7 to
2c723f0
Compare
Fix for issue: #306