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

Allow overriding Promise library #265 #599

Closed
wants to merge 33 commits into from

Conversation

Worthaboutapig
Copy link

This is done, however, there are some failing test on apparently unrelated stream issues.
There's a few changes to make tests etc easier to run under an IDE.
(Updated packages and uses pnpm)

Base automatically changed from master to main February 17, 2021 22:01
@cressie176
Copy link
Collaborator

Thank you for the PR and sorry it has taken so long to respond. I'm closing in favour of native promise support now that support for older node versions has been dropped.

@cressie176 cressie176 closed this May 15, 2022
@Worthaboutapig
Copy link
Author

Thank you for the PR and sorry it has taken so long to respond. I'm closing in favour of native promise support now that support for older node versions has been dropped.

No problem. The approach I took handled this by using the native Promise library by default, but allowing people to pass in their own if they liked. This seems the most flexible option.

Is there an issue for the changes, are they in progress? My PR works fine in practice (though appreciate isn't necessarily suitable for merging).

@cressie176
Copy link
Collaborator

Hi @Worthaboutapig, I thought about whether we want people to be able to supply their own promise library and decided against it. I agree it would be more flexible, but the internals of the library should be opaque to users, and it would create potential for some difficult to diagnose bugs if we allowed people to specify their own.

#627 exists and there is also a PR #624

At the moment I'm picking off some smaller/simpler changes though.

@Worthaboutapig
Copy link
Author

Ok, thanks. I made the changes because Bluebird created a bunch of problems in debugging, as it throws loads of exceptions as part of its normal startup behaviour. It was horrendous to the point of making my whole debugging experience with RabbitMQ based stuff unusable- so IMHO it's critical to being useful.
Perhaps others don't have the same debugging experience I do though.

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.

2 participants