Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

introduce native promise support incl. documentation & types #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmillr
Copy link

@tmillr tmillr commented Aug 5, 2021

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 tmillr marked this pull request as draft August 5, 2021 11:43
@tmillr
Copy link
Author

tmillr commented Aug 5, 2021

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
Copy link
Author

tmillr commented Aug 5, 2021

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.

@tmillr tmillr marked this pull request as ready for review August 6, 2021 10:00
@tmillr tmillr changed the title add native promise support incl documentation introduce native promise support incl. documentation & types Aug 6, 2021
@tmillr tmillr force-pushed the master branch 3 times, most recently from 9bf0f43 to cb53627 Compare August 10, 2021 23:44
@aurbano
Copy link
Owner

aurbano commented Oct 15, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants