Closed fatso83 closed 6 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
93db3ef
) 96.02% compared to head (9f195f6
) 96.02%.:exclamation: Current head 9f195f6 differs from pull request most recent head 5fdc8e8. Consider uploading reports for the commit 5fdc8e8 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I am partial to closing this in favor of just improving the documentation with more examples. The API has been stable for ages and is well documented: https://sinonjs.org/releases/v17/stubs/#stubthrows, so I think this just comes down to some unfortunate tooling effects.
Right now, it seems that tooling (VSCode, etc) has a hard time dealing with typescript definitions from one source and JSDoc strings from another, so people might lose out on the actual docs. Not sure how to deal with this ATM, unfortunately, but it's really another issue.
Any thoughts, @JemiloII and @dgreenwald-ccs (seeing that you commented in the original issue).
When I made that comment years back, promises were fairly new. A lot has changed and now we have async/await to avoid some the issues we had before. So having the docs with more examples would be very helpful so if someone does get confused, they can see it in the docs
Purpose (TL;DR) - mandatory
Fix #1679 by changing the Stub and Fake API to not wrap strings in Error
Background (Problem in detail)
In #1679 it was raised how stub.rejects("error") does not reject the promise with the passed in reason, but chooses to wrap it in an error. Some people found that behavior surprising that and had objections to that API design.
@mantoni chimed in to say that he thought stub.rejects should behave in line with
Promise.reject
.I made the point that while
stub.rejects(string)
might be surprising to someone comparing it to thePromise.reject
API _it is totally consistent with the current behavior ofstub.throws(string)
and that we should keep the two aligned.Solution
This is one solution to the issue: it changes the
rejects
andthrows
API's of Stub and Fake to not treat the single-argument string in a special matter. This will potentially break a lot of existing code, but at least keeps consistency across the API's for throwing sync and async.There are other takes, one being to simply keep things as they are and document in the docs how to these are utility methods to achieve the most normal flow (which is throwing an error sync and async), and simply update the docs (both JSDoc and homepage docs) to point to other methods for achieving more specialized cases.
For instance, if you want to throw a string using the API that is today (before this change), you can just create a fake or stub that does that:
Same goes for Promises / throwing async
I think this is just as valid an approach: making people aware that the basic building blocks are there. That's what I like about the Fake API: fewer methods in that API to rather create explicit behavior that creates less misconceptions about what something is doing.
How to verify - mandatory
npm install
Checklist for author
npm run lint
passes