sinon: make calledWith partial

[43081j]

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).
• Provide a URL to documentation or source code which provides context for the suggested changes: https://sinonjs.org/releases/v7.3.0/spies/

---

Fixes #33839

Summary of the mass of posts below & the changes here:

• SinonInspectable isn't a real thing, sinon has no such interface/concept, it has been removed from the types
• SinonSpy when used in spread has been changed to use any as TArgs (rather than any[], the default) to allow having a collection of spies (trade-off we must make to avoid introducing a workaround interface like the whole SinonInspectable thing did)
• calledWith is partial, so can accept some of the arguments rather than requiring them all
• the inspectable changes introduced a breaking change we overlooked (a big one which caused thousands of type errors in my case), removing it and breaking whoever uses it is a safer play than leaving everyone else broken

[typescript-bot]

@43081j Thank you for submitting this PR!

🔔 @MrBigDog2U @rationull @lumaxis @nicojs @JoshuaKGoldberg @gjednaszewski @johnjesse @alecf @SimonSchick @bergundy - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[typescript-bot]

@43081j The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[43081j]

fyi build fails because the recent change to add nested matchers results in an inferred assertion type of any.

looking into it

[43081j]

@felixfbecker do you mind explaining your inspectable interface that was added not so long ago?

did you introduce it so you could pass an array of spies? or was it for some other reason?

basically it is causing trouble because TS3.1 can't infer the types correctly whereas 3.2 can.

surely, if it was to solve the "array of spies" problem, SinonInspectable should be a non-generic base interface all spies implement (i.e. a SpyLike type) so you can correctly pass an array of them without caring what the generic types are.

[typescript-bot]

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

[typescript-bot]

@43081j The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[43081j]

alright here's a proposed solution (latest commit).

• reverted SinonInspectable because im not sure what you were trying to solve with that
• introduced SinonSpyLike and SinonSpyCallLike which are non-generic bases of each interface
• changed each method which doesn't care about the type of spy to take the non-generic bases instead
• changed any occurrences of arrays of spies to be arrays of the bases

In strongly typed languages you'd usually do something like this (which isn't possible in TS):

interface Foo {
  prop1: string;
  prop2: any;
}

interface Foo<T> extends Foo {
  prop2: T;
}

to get around the fact we can't quite do this, i named it instead SpyLike rather than it being a non-generic Spy.

This will be a breaking change in that the SinonInspectable interface is gone, but I suspect the adoption/usage of that specific interface is tiny being that it was only merged very recently.

it is a bit incorrect though, as it now means the non-generic spies don't inherit SinonSpyApi. that doesn't mean much as people shouldn't really be consuming these base interfaces, but it does make it a bit weird. seeing as you could technically pass a spy in which has no "api" methods on it now...

let me know what you think

DT maintainers DO NOT MERGE YET as i need feedback on this change (@SimonSchick and @felixfbecker if possible)

[felixfbecker]

The SinonInspectable was added because the assert functions do not need to use the call signature of the spy (it only asserts the recorded calls, never calls the spy). If the call signature was in there, passing a concrete SinonSpy<TArgs, TReturn> (where TArgs and TReturn are not any) would (rightfully) throw a type error (it was not for passing an array of spies). This solution was chosen as the best after discussion on #33201. The PR at some point had just widened the constraint to any everywhere, but you (rightfully) said that's not the nicest solution. It seems this PR basically now reverts to that. Except I think it's not needed to have a separate interface for that, you could just make the type parameters any.

I don't really understand why SinonInspectable causes a problem with making calledWith partial. Could you give an example?

[43081j]

Except you can't. A Spy<any> isn't the same as a Spy<somethingElse>. these are two distinct types.

This is exactly why in most strongly typed languages, you have a non-generic base with generic sub classes. The same reason the array has problems, too.

My change to make calledWith accept a partial is unrelated to the rest of the changes. The other changes were because the typescript versions DT builds against have presumably changed since the last PR was merged, so the untouched sinon types fail in 3.2 but not 3.3.

They fail because the recursive match arguments type results in any being inferred, allowing spies to be called with anything.

While 3.3 and above seem to infer this correctly, it is still wrong. Better to rely less on an edge case of inference and instead just write the types a bit better.

Additionally, the SinonInspectable types fail in one of the versions too for a similar reason:

https://travis-ci.org/DefinitelyTyped/DefinitelyTyped/builds/510373241?utm_source=github_status&utm_medium=notification

I still don't fully understand your train of thought. SinonInspectable is its self generic.. so it will have many of the same problems SinonSpy has, surely.

[43081j]

In your PR that introduced the inspectable stuff, things seemed fine until you wanted an array of spies.

You then introduced the inspectable stuff. So I suspect the only reason you really needed any of this was so you can have a mixed array of generics, in which case it was the wrong way to do it and im surprised it worked at all (i guess the any turned type checking off so it 'worked').

I'm just a little confused as to what it was for. Would you mind giving an example?

[typescript-bot]

@43081j The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[typescript-bot]

@43081j I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

[43081j]

@sandersn @sheetalkamat can one of you take a look at this?

its becoming pretty aged but the problem still needs resolving and a cleanup of the now-cluttered interfaces would be nice.

i've bumped into this problem (in the build) a lot, too, so whatever happens with this PR it would still be of great use to understand the general solution to it (single-use type params for handling tuples).

if it turns out it needs changes in dtslint, ill do it. but it seems like there should be a solution without changing that.

[typescript-bot]

@43081j The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

[sandersn]

@43081j simple suggestion before I finish reading the entire history: disable the lint rule that forbids single-use type parameters. tslint lets you disable rules for a single line.

[43081j]

@sandersn if you don't mind that i will. i just thought it would be a bit hacky as i wasn't sure if the rule is in fact right and im doing something wrong.

ill update

if i have to disable the rule, does that mean the rule has a bug for this edge case? im happy to fix it if there is one

i've also added a summary for you in the original post at the top

[rbuckton]

The overall problem that SinonInspectable attempted to solve was one of variance. By having both a call signature of TArgs as well as various methods that both accept and return something with TArgs, the type for TArgs would become invariant in SinonSpy. When we measure TArgs as invariant, a SinonSpy would only become assignable to another SynonSpy where TArgs is assignable in both directions. Unfortunately any[] (the default for TArgs) is not assignable to any tuple.

However, any (rather than any[]) is assignable in both directions, therefore SinonInspectable isn't precisely necessary, but rather any instance of SinonSpy where we don't care about the actual type of TArgs we can replace with any (rather than any[]), and assignability should work correctly.

[43081j]

@rbuckton this is excellent, much appreciated taking the time to look through. i've made changes you suggested

makes a lot more sense now that we should be using any on the way in as it doesn't make a difference to the return type, but can still use TArgs in places where we want to return a spy of the same type.

this has sat around for a long time and is why many people still haven't updated their types so it will be great to finally get it agreed on

[felixfbecker]

Thanks @rbuckton that is 100% accurate, I was unable to get the reason for SinonInspectable across. I am fine with any too, my initial PR that added SinonInspectable actually used any first for that exact reason but I changed it because of PR feedback.

[43081j]

It was already put across and well understood but was an unnecessary interface which didn't exist in sinon its self and was mostly to work around the array of generics in the end. there were various reasons the original attempt of using any wasn't ideal from what I remember.

master currently has a rather bad breaking change preventing an update because of this workaround interface so if we're all happy with the new implementation that is great.

would be good to get it merged

[typescript-bot]

🔔 @felixfbecker @rbuckton - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

[typescript-bot]

I just published @types/sinon@7.0.12 to npm.