Skip to content
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

feat: drop bluebird for native promises #70

Closed
wants to merge 3 commits into from

Conversation

FauxFaux
Copy link

@FauxFaux FauxFaux commented Apr 7, 2020

Bluebird isn't really doing anything for us, except messing up async hooks and stack traces in my app.

BREAKING CHANGE: all methods' return types changed, and disposer-based interface is just gone.


We could avoid that p-map dependency by just using Promise.all, which is pretty much all it's doing anyway.

nodeify needs to not be a monkeypatch, but the diff is easier to read here. I'll fix it properly. Personally I'd just totally drop callbacks, but doing so causes the tests to go nuts in some way I'm totally failing to debug.


Thoughts?

@coveralls
Copy link

coveralls commented Apr 7, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 741f528 on FauxFaux:feat/bluebird into 870778c on mike-marcacci:master.

@mike-marcacci
Copy link
Owner

This is great - I've been unable to give this library the attention I would like recently, but removing bluebird has been on my list for a while.

I have a use-case that will require me to spend a bit more time on this library over the next week or so. After a cursory look this seems great 👍 so I'll probably merge it before I get started on my changes.

If you do have a chance to remove p-map that would be even better and make the library zero-dependency; if not, no worries: I'll do this after merging.

Thanks for the PR!

@FauxFaux
Copy link
Author

FauxFaux commented Apr 8, 2020

I took out p-map (for Promise.all), and made nodeify a top-level function, in a way that makes the formatting only slightly weirder.

Feel free to cherry-pick or rewrite if you don't like how it's turned out.

@FauxFaux FauxFaux marked this pull request as ready for review April 8, 2020 16:25
BREAKING CHANGE: all methods' return types changed, and disposer-based
interface is just gone.
@FauxFaux
Copy link
Author

FauxFaux commented Dec 1, 2020

@mike-marcacci: I'm still hoping this might be mergable? I resolved the merge conflicts.

Are you particularly attached to the disposable interface? To bluebird?

@mike-marcacci
Copy link
Owner

Hi @FauxFaux, so sorry for the delay here; I am not opposed to this at all and no longer have any need for anything bluebird provides here.

I have been hoping to incorporate this change into a major revision that's just been languishing, and which I finally pushed as #84. My hesitation here isn't in merging this but releasing it. This is a breaking change and I'm a bit reluctant to have multiple major releases in close succession, due to the upgrade burden that places on all library consumers.

@mike-marcacci mike-marcacci mentioned this pull request Dec 12, 2020
9 tasks
@FauxFaux FauxFaux mentioned this pull request Apr 7, 2021
Closed
@gsouf
Copy link

gsouf commented Apr 29, 2021

For me bluebird is a biger burden than upgrading node-redlock and deal with the breaking changes :)

@simonemazzoni
Copy link

@mike-marcacci any chance of having this released? I agree with the others that Bluebird introduces unnecessary complexity for no reason. I understand your concern of maintaining two major versions, but I also think that update to a new version is not that complicated for the users of the library. I would prefer not having to fork the library.

@gsouf
Copy link

gsouf commented Jun 16, 2021

Anyway who uses bluebirds nowadays except people who didnt upgrade their software since the stone age of promises? Getting rid of these archaic things is a favor you make to people who use your library :D

@mike-marcacci
Copy link
Owner

Hey folks! I'm going ahead and closing this, as this is addressed in v5 (currently in beta).

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

Successfully merging this pull request may close these issues.

5 participants