Enable use of assignment in the presence of accessors

[fatso83]

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 into the root of the repo, then run node *.test.mjs.

Checklist for author

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

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f8c20e5) 95.95% compared to head (b5bd9b1) 96.03%.

❗ Current head b5bd9b1 differs from pull request most recent head 3a8eaf8. Consider uploading reports for the commit 3a8eaf8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2538      +/-   ##
==========================================
+ Coverage   95.95%   96.03%   +0.07%     
==========================================
  Files          40       40              
  Lines        1904     1916      +12     
==========================================
+ Hits         1827     1840      +13     
+ Misses         77       76       -1     

Flag	Coverage Δ
unit	96.03% <100.00%> (+0.07%)	⬆️

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

Files	Coverage Δ
lib/sinon/sandbox.js	97.89% <100.00%> (+0.55%)	⬆️

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

[mantoni]

Looks good. While the name is okay, I'm also wondering about alternatives.

Suggestion: accessorInjection or useAccessorInjection

Another question: should the whole thing fail with a specific error message if the flag is specified, but no accessor pair is found?

[mantoni]

One more thought: Since this is a completely different approach to swapping out a function, we could also introduce sandbox.inject(...) to make it a bit less verbose. WDYT?

[fatso83]

Maybe that's just as good? We would then have define, inject and replace, all with dedicated use cases.

[fatso83]

I had just about finished the changes when I found that we already have Sandbox#inject, doing something completely different (injecting a subset of the sandbox's functionality on an object). Pretty sure it is not the most well-known function and frankly I am not totally sure what its usecase outside of internal use. We even do not have any documentation on it and it is only used once (in lib/sinon/create-sandbox.js (line 53)). All of that sandbox setup related code is basically shrouded in mystery by now, so I have no idea how all of this works.

So ... I need a new name.

• #accessorAssign - too brief?
• #injectUsingAccessor - overloading the term "inject", confusing readers when comparing to the existing mentions of injectTo and inject()
• #forceAssign - I think the below gives more hints?
• #forceAccessors - will probably make one wonder what it does, but clearer on first reading of docs?
• #assignUsingAccessors() - current favorite. Quite verbose, but clear.

Currently voting for sinon.assignUsingAccessors(obj, 'doFoo', fake). Whatcha think? 🤷

#naming_is_hard

[mantoni]

I agree that sinon.assignUsingAccessors(obj, 'doFoo', fake) is the most clear. Quite a handful to type though, but I'm also running out of ideas.

The very last thing I can thing of: I checked the work on sinon.define(...) and it uses assignments, if I understand correctly. So the use case here would work with it, however I don't think the restorer would. Do you think it's worth trying to fold this functionality into define? Or do you prefer keeping them separate? I don't have a strong opinion here.

[fatso83]

I am just thinking sinon.define has a semantic meaning: it is supposed to create something where there is nothing. So there is not much overlap in intent. I don't mind sharing parts of the implementation on the inside, but I want the naming to be clear.

What about ... namespacing it? It is kind of a special case of sinon.replace, so one could make the name itself slightly less verbose through sinon.replace.usingAccessors(...)? Might be putting lipstick on a pig, as they say over there ...

[mantoni]

I agree with the intent argument. It's a different thing and we should probably not mix them

I'm not really happy with the this, but can't think of anything better. Maybe @mroderick has more ideas?

[mroderick]

I'm not 100% sold on having a fourth argument with an obscure name that people have to remember in order to replace accessors... especially considering we already have sinon.replaceGetter and sinon.replaceSetter.

If we adopt the fourth argument, should we then deprecate replaceGetter and replaceSetter?

[fatso83]

I think we already agreed to forego the fourth argument. The discussion now is basically about which dedicated name to use.

This is not about replacing accessors, Morgan, so the existing functions for replacing accessors should stay as they are. This is just about assigning values using the accessors, without replacing them. See the expanded discussion.

[mroderick]

@hexeberlin and I had a look at this, and now I think I'm actually caught up and understand what's going on.

How about sinon.replace.accessor(...)?

We can modify sinon.replace to mention that method, when it throws on trying to replace an accessor and we can document our way out of it, so that Future Developer will have a chance to understand what Current Author intended with using sinon.replace.accessor.

Thoughts?

[fatso83]

I like it! Makes it clear that it is a special case. I can just update this and get this merged if there's agreement.

[mroderick]

I can just update this and get this merged if there's agreement.

Go ahead!

[fatso83]

When actually writing it out, I had second thoughts about replace.accessor, since it sounds like you want to replace the accessor, so instead I resorted to replace.usingAccessor. I think it both reads better and makes more sense. Should be ready to merge, unless anyone wants more unit tests. I just copied the assertions from the normal .replace, but I haven't added additional checks for the method reusing them.

[mantoni]

Just one tiny comment. 👍