Stop wrapping strings in Error

[fatso83]

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 the Promise.reject API _it is totally consistent with the current behavior of stub.throws(string) and that we should keep the two aligned.

Solution

This is one solution to the issue: it changes the rejects and throws 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:

sinon.stub(() => {throw "foo"; })
// throws a string "foo"

Same goes for Promises / throwing async

sinon.stub(() => Promise.reject("foo"))
// alternatively
sinon.stub().returns(Promise.reject("foo"))

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

1. Check out this branch
2. npm install

Checklist for author

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (93db3ef) 96.02% compared to head (9f195f6) 96.02%.

❗ Current head 9f195f6 differs from pull request most recent head 5fdc8e8. Consider uploading reports for the commit 5fdc8e8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2569      +/-   ##
==========================================
- Coverage   96.02%   96.02%   -0.01%     
==========================================
  Files          40       40              
  Lines        1912     1911       -1     
==========================================
- Hits         1836     1835       -1     
  Misses         76       76              

Flag	Coverage Δ
unit	96.02% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
lib/sinon/default-behaviors.js	100.00% <100.00%> (ø)
lib/sinon/fake.js	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fatso83]

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).

[JemiloII]

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