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

chore(promises): removed support for custom promise libraries #12878

Merged
merged 6 commits into from Feb 4, 2023

Conversation

lpizzinidev
Copy link
Contributor

closes #12872

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test/connection.test.js we use the package q and bluebird. I wonder if we can remove those tests too? @vkarpov15

@@ -35,7 +34,7 @@ module.exports = function promiseOrCallback(callback, fn, ee, Promise) {
}
}

Promise = Promise || PromiseProvider.get();
Promise = Promise || global.Promise || require('bluebird');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if bluebird is not installed? Crash gracefully?

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good to me, though there are some test things that should maybe be changed

also i think this change should target the next major version instead of master (/ 6.x)

test/docs/promises.test.js Show resolved Hide resolved
@lpizzinidev lpizzinidev changed the base branch from master to 7.0 January 8, 2023 08:48
test/docs/promises.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

docs/promises.md Outdated Show resolved Hide resolved
lib/helpers/promiseOrCallback.js Outdated Show resolved Hide resolved
test/docs/promises.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, i think that bluebird and q can now be removed from the package.json

@vkarpov15 vkarpov15 merged commit 794ee88 into Automattic:7.0 Feb 4, 2023
@CutiePi
Copy link

CutiePi commented Apr 14, 2023

Maybe add it as an inclusion to the migration DOC 😅 although it is in the changelog.

I went from v5 -> v7 and found out the hard way lol

I found this because of that commit message on the main page
image

@hasezoey
Copy link
Collaborator

just as a note, if you still want to use custom promises, you should be able to do:

global.Promise = YourCustomPromise;

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

Successfully merging this pull request may close these issues.

Remove support for custom promise libraries
5 participants