aurbano / robinhood-node

:chart_with_upwards_trend: NodeJS client for Robinhood Trading :fire:
https://aurbano.github.io/robinhood-node
MIT License
694 stars 185 forks source link

introduce native promise support incl. documentation & types #137

Open tmillr opened 2 years ago

tmillr commented 2 years ago

Added native promise support for consumers of this module.

Already updated README.md to include examples and some other stuff.

Also did some very basic tests on my own in the node repl (although I have not included any written tests in this PR)... this is a rather simple update and everything appears to check out.

Requires recent version of Node due to usage of the package.json "exports" field (which is itself backwards-compatible since I left "main" in there).

The promise support is implemented via a wrapper in its own separate file (./src/promises.js), so this should really not be a breaking change. The original api was left untouched.

The only thing that seems unclear to me is error-handling when it comes to this new promise version of the api, and which errors are catchable and which aren't. At the very least, I am fairly confident that any thrown errors will not go unnoticed. I did see some errors being thrown, instead of returned via callback, in async callback contexts in some of the login/init methods of this module. I'm pretty sure those cannot be caught no matter if a consumer is using this new promise api nor the original callback api. Other than that, Promises + Errors = Confusion (at least for me), but it still seems like everything should be just fine.

Should be all good to go!

tmillr commented 2 years ago

Edit: changed to draft until me or someone else can implement types for this addition...

here seems useful for this: https://github.com/teppeis/typescript-subpath-exports-workaround

tmillr commented 2 years ago

Added types for the new promise api and moved all types to their own types dir. PR is officially ready! :D

There is one typescript caveat to mapping types for subpath package imports/exports this way (via typesVersions package.json key) in order to mirror the behavior of, and support, the package.json exports key which node uses at runtime for resolution , and that is that relative path references to subpaths in the package ("./robinhood/promises") will not be able to find or correctly map to the types that we have mapped in package.json for some reason ("./robinhood" & "robinhood/promises" both work fine however). For some reason, ts only seems to consult typesVersions when the import is a regular package reference as opposed to a relative path reference, so this typescript caveat only applies to subpath imports (e.g. "./robinhood/promises") that are also relative path references (relative filesystem paths), and for now that means that it only applies to the promises subpath since that is the only exported subpath of this module right now. Most people are probably going to be installing this as a regular node module and are probably not going to be importing via relative paths anyway.

The only other thing that might be a good idea to do is update the typescript dev-dependency since one of the .d.ts files now requires typescript 4.1 or higher (I configured a fallback in package.json for typescript consumers/users of this module utilizing older versions of ts). Idk how dependency updates are being handled in this repo however, so I'm not touching that.

aurbano commented 2 years ago

This looks like a great addition @tyler1205 !

I'll look through it a bit more carefully soon, although I'm thinking that it might be time to make the library use TypeScript natively, and include promise support in all methods, so they can be used either with callbacks or promises.