-
Notifications
You must be signed in to change notification settings - Fork 750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use native promises #439
Use native promises #439
Conversation
12ccdf2
to
61cd3ca
Compare
Awesome! I'm just thinking about the idea of allowing the library's With that assumption, we may even want to not expose a way to configure a promise if possible so that we won't have to deal with that additional API over the long term. I wonder if we should hide that configuration for now, and then add it in if/when someone asks for it? |
TBH, I don't either 😆 That said, I've done some quick research and it seems people prefer third-party libraries like Bluebird over native promises for two reasons:
In our case, performance is probably a non-issue (the time spent in the Promise implementation is likely negligible compared to the time waiting for network requests to complete), and this PR proves we can get by with the native promises API. (Of course, users can still use third-party promise libraries in their own code.) So, yeah, it probably makes sense for us to remove the |
61cd3ca
to
6de1da1
Compare
6de1da1
to
722a12f
Compare
Haha, thanks OB. I just hope that I wasn't too far off point and that we don't just have to bring this back to your original implementation in a week ;) LGTM. |
I have run the latest benchmark on my laptop (./bench doxbee):
|
@OrKoN Thanks for doing that! Looks liike amazingly, Bluebird is still faster. I wouldn't have thought that the native promises would have so many more opportunities for optimization compared to a userspace library. |
r? @brandur-stripe
cc @stripe/api-libraries @OrKoN
Add asetPromise
configuration option for users to provide their ownPromise
implementationFixes #438.