Enable use of assignment in the presence of accessors

fatso83 commented 11 months ago

Purpose

Allows specifying a fourth argument, an options bag, to sandbox.replace() to make it possible to override the default behaviour of throwing when encountering accessor methods (getters/setters).

This means we will pass the supplied replacement to the setter and vice-versa get the original value from the getter when restoring.

P.S. I am not sold on the options name, so could be something else like "allowUsingAccessor". Was just inspired by the original issue.

Background (Problem in detail) - optional

Issue #2403 made the case for using assignment, instead of defining object descriptors, in injection methods. Enabling this in all existing methods would probably break some projects in unforeseen ways. It would also mean adding yet another argument to most of these, which would make them harder to use, as some of them (spy) already have optional third arguments today.

The discussion in #2403 revealed we already had a "special case" in our Sandbox#replace method, as this already uses basic assignment. The only thing preventing it from being used was that it tries to prevent false usage by throwing on encounter accessor methods for the given property. Thus this PR simply opens up for circumenting this.

This opens up for some exotic cases, such as manual setups for stubbing the exports of true ESM modules through the innards of the module, in the way described in that feature:

my-module.mjs

export function doThing() {
    console.log("Wow, we did the thing!");
}

export function doThingTwice() {
    doThing();
    doThing();
}

export const myModule = {
    get doThing() {
        return doThing;
    },
    set doThing(value) {
        doThing = value;
    },
};

my-module.test.mjs

import { doThingTwice, myModule } from "./my-module.mjs";
import sinon from "./lib/sinon.js";

const stub = sinon.replace(myModule, "doThing", sinon.fake(), {
    forceAssignment: true,
});

doThingTwice();

sinon.assert.calledTwice(stub);

Alternatively one could, of course, not do this change, and instead say that this manual stubbing setup is already doable by exposing functions for setting the corresponding exports and manually invoking these. The downside is that this will cause lots of manual restore code, which would otherwise by handled by Sinon.

How to verify - mandatory

Run tests and alternatively try the copy-pasting the example above. You will typically need to say import sinon from '../lib/sinon-esm'.

Checklist for author

[x] npm run lint passes
[x] References to standard library functions are cached.

codecov[bot] commented 11 months ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (fe799e7) 95.73% compared to head (96255d9) 95.74%.

:exclamation: Current head 96255d9 differs from pull request most recent head 91036eb. Consider uploading reports for the commit 91036eb to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2537 +/- ## ======================================= Coverage 95.73% 95.74% ======================================= Files 40 40 Lines 1901 1905 +4 ======================================= + Hits 1820 1824 +4 Misses 81 81 ``` | Flag | Coverage Δ | | |---|---|---| | unit | `95.74% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files Changed](https://app.codecov.io/gh/sinonjs/sinon/pull/2537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs) | Coverage Δ | | |---|---|---| | [lib/sinon/sandbox.js](https://app.codecov.io/gh/sinonjs/sinon/pull/2537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs#diff-bGliL3Npbm9uL3NhbmRib3guanM=) | `95.59% <100.00%> (+0.07%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fatso83 commented 11 months ago

Wrong name of branch. Closing and reopening

sinonjs / sinon