Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@types/sinon] sinon.spy lacks the third argument #32398

Closed
3 of 4 tasks
karak opened this issue Jan 23, 2019 · 11 comments
Closed
3 of 4 tasks

[@types/sinon] sinon.spy lacks the third argument #32398

karak opened this issue Jan 23, 2019 · 11 comments

Comments

@karak
Copy link
Contributor

karak commented Jan 23, 2019

If you know how to fix the issue, make a pull request instead.

If you do not mention the authors the issue will be ignored.

We can't spy accessors because the current definition of sinon.spy lacks third argument called types, which should be typed as types?: string[].

See https://github.com/sinonjs/sinon/blob/master/lib/sinon/spy.js.

My test project is here: https://codesandbox.io/s/92n50m2n1p.

@nicojs
Copy link
Contributor

nicojs commented Jan 23, 2019

Is this an undocumented feature? I don't see it here: https://sinonjs.org/releases/v7.2.3/spies/#creating-spies-sinonspy-method-signatures

Generally, we only add typings of features that are documented.

@43081j
Copy link
Contributor

43081j commented Jan 23, 2019

It is undocumented.

I suggest opening an issue with sinon to find out if it's supported (as in they won't just remove it one day and it isn't deprecated).

@karak
Copy link
Contributor Author

karak commented Jan 23, 2019

OK. I issued.

@karak
Copy link
Contributor Author

karak commented Jan 24, 2019

It's supported though still undocumented.
sinonjs/sinon#1970 (comment)

@nicojs
Copy link
Contributor

nicojs commented Jan 24, 2019

Cool, feel free to create a PR (for both sinon 🚀 and definitely typed 🚀 )

@karak
Copy link
Contributor Author

karak commented Jan 24, 2019

Is that documentation guarding?
Actually, I had already had a type patch in my(our) local, but no works of sinon itself.

@karak
Copy link
Contributor Author

karak commented Jan 28, 2019

Oh, I found this fix is incomplete!

If types is used, the return-value is a plain object that has specified properties with Spy object: see https://github.com/sinonjs/sinon/blob/master/lib/sinon/spy.js#L53

In example:

  const descriptor = sinon.spy(RGBColor.prototype, 'red', ['set', 'get']);
  const color = new RGBColor();
  color.red = 128;
  expect(descriptor.set.callCount).toBe(1);

I'd like to have official documentation.

@43081j
Copy link
Contributor

43081j commented Jan 28, 2019

All the line you linked to is doing is setting the descriptor config (as opposed to an empty, default descriptor). This is just so it sets the getter/setter correctly through the property descriptor.

It should just return a spy like any other spy call.

Your example works because sinon returns the wrapped method, which in this case is just the property descriptor its self.

@karak
Copy link
Contributor Author

karak commented Jan 29, 2019

Ah, you mean a return value should have the type of the original method, property descriptor in this case, instead of just a callable, then?

EDIT: And set/get of those properties must be sinon spy as well.

@43081j
Copy link
Contributor

43081j commented Jan 29, 2019

alright so the short explanation to this is that sinon does some hacky things when you spy on something.

behind the scenes, it just attaches a bunch of sinon things to the property descriptor. the descriptor is usually {} so you don't notice anything extra, but when you use ['get', 'set'], it is {get: fn, set: fn}. Meaning technically a spy with get/set does have a get and set property (because again it is just the descriptor with sinon stuff added to it).

we can't easily represent this in strong types, though... its doable in some way i imagine but could lead to unnecessary complexity. if its wanted enough, someone should take a stab at it but otherwise i'd personally leave it out (and somehow work around it).

@karak
Copy link
Contributor Author

karak commented Jan 30, 2019

Agree. The typing would be hard and complex.

Can I show my image by code bellow? I know the descriptor is just a definition, but I can't grab the final type of that execution pass.

karak@95c18e5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants