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

Add usingPromise() method on fakes to fix issue #2293 #2301

Merged
merged 2 commits into from Oct 13, 2020
Merged

Add usingPromise() method on fakes to fix issue #2293 #2301

merged 2 commits into from Oct 13, 2020

Conversation

romanbalayan
Copy link
Contributor

Add usingPromise() method on fakes

Fix issue #2293. Added implementation of usingPromise() to be able to override global Promise library.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. sinon.fake().usingPromise(bluebird).resolves(42);

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this! Just a few quibbles on the implementations. Not seeing why you want to handle the native and the injected implementation as two different things, so you could simplify it.

throw new TypeError("Expected f argument to be a Function");
}
function fakeClass() {
var promiseLib;
Copy link
Contributor

Choose a reason for hiding this comment

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

If they always have the same interface, why bother with branching?

Suggested change
var promiseLib;
var promiseLib = Promise;

Comment on lines 69 to 71
if (promiseLib) {
return promiseLib.resolve(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (promiseLib) {
return promiseLib.resolve(value);
}

Comment on lines 80 to 82
if (promiseLib) {
return promiseLib.reject(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (promiseLib) {
return promiseLib.reject(value);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fatso83,

That actually makes a whole lot of sense, will take those edits. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also noticed that i'm not wrapping the the rejected object as an Error. Will correct that as well.

@fatso83
Copy link
Contributor

fatso83 commented Oct 13, 2020

This is great, thanks a lot, @romanbalayan!

@fatso83 fatso83 merged commit 3b80dbb into sinonjs:master Oct 13, 2020
@romanbalayan
Copy link
Contributor Author

Hi @fatso83,

I'm also currently participating in the hacktoberfest event organized by Digital Ocean, hopefully it would not be much of a bother for you to add the hacktoberfest-accepted label to this PR so it may be counted towards my progress?

Thank you!

@fatso83
Copy link
Contributor

fatso83 commented Oct 13, 2020

Done!

@mroderick
Copy link
Member

Great PR! Thank you @romanbalayan

@mroderick
Copy link
Member

This has been published with sinon@9.2.1

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.

None yet

3 participants