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

Breaking change in 10.0.1: config.target is no longer supported #2352

Closed
kevinoid opened this issue Apr 8, 2021 · 17 comments
Closed

Breaking change in 10.0.1: config.target is no longer supported #2352

kevinoid opened this issue Apr 8, 2021 · 17 comments

Comments

@kevinoid
Copy link

kevinoid commented Apr 8, 2021

Describe the bug
With sinon@10.0.1, if options contains target, sinon.useFakeTimers throws a TypeError. This did not occur with sinon@10.0.0.

To Reproduce

Create index.js with the following content:

const sinon = require('sinon');
sinon.useFakeTimers({ target: { Date } });

Run node index.js after npm install sinon@10.0.0 and after npm install sinon@10.0.1. After installing sinon@10.0.1 the following error is printed:

/path/to/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:1299
            throw new TypeError(
            ^

TypeError: config.target is no longer supported. Use `withGlobal(target)` instead.
    at Object.install (/path/to/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:1299:19)
    at createClock (/path/to/node_modules/sinon/lib/sinon/util/fake-timers.js:12:31)
    at exports.useFakeTimers (/path/to/node_modules/sinon/lib/sinon/util/fake-timers.js:54:16)
    at Sandbox.useFakeTimers (/path/to/node_modules/sinon/lib/sinon/sandbox.js:396:46)
    at Object.<anonymous> (/path/to/index.js:2:7)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)

Expected behavior
No exception would occur.

Additional context
The problem was introduced by f838d78 which bumped @sinonjs/fake-timers from ^6.0.1 to ^7.0.4 which includes sinonjs/fake-timers#318.

@regseb
Copy link

regseb commented Apr 8, 2021

I have a problem with sinon@10.0.1 and typescript which is surely related to this issue.

To reproduce, create index.js with the following content:

const sinon = require("sinon");

And execute:

npm install sinon@10.0.1
npm install @types/sinon@9.0.11
npm install typescript@4.2.4
npx tsc index.js --declaration --allowJs --emitDeclarationOnly

The result is:

node_modules/@types/sinon/index.d.ts:778:36 - error TS2694: Namespace '"/tmp/testcase/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'TimerId'.

778     type SinonTimerId = FakeTimers.TimerId;
                                       ~~~~~~~

node_modules/@types/sinon/index.d.ts:780:39 - error TS2694: Namespace '"/tmp/testcase/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'InstalledMethods'.

780     type SinonFakeTimers = FakeTimers.InstalledMethods &
                                          ~~~~~~~~~~~~~~~~

node_modules/@types/sinon/index.d.ts:781:20 - error TS2694: Namespace '"/tmp/testcase/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'NodeClock'.

781         FakeTimers.NodeClock &
                       ~~~~~~~~~

node_modules/@types/sinon/index.d.ts:782:20 - error TS2694: Namespace '"/tmp/testcase/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'BrowserClock'.

782         FakeTimers.BrowserClock & {
                       ~~~~~~~~~~~~


Found 4 errors.

@fatso83
Copy link
Contributor

fatso83 commented Apr 8, 2021

@regseb we don't maintain those typescript bindings, but ship our own. Ah, sorry, I saw the problem is the fake-timer definitions which we DO maintain. This change needs to be reverted.

@0xSage
Copy link

0xSage commented Apr 8, 2021

Hi thanks for fixing. what's the ETA on this?

We'll pin our version, if it's going to take a few days.

@fatso83
Copy link
Contributor

fatso83 commented Apr 9, 2021

@nicoleczhu pin your version. no one is paid to do this, so it will be when someone does the work to fix and test this manually. that being said, I think this has a high priority, so it will be looked at soon by someone in @sinonjs/sinon-core

@bcoe
Copy link

bcoe commented Apr 9, 2021

@fatso83 I empathize with the no one is paid to do this argument, as someone who's sunk countless hours into handling CVEs for injection bugs logged against yargs.

But just wanted to make sure you knew about this feature on npm:

npm dist-tag add sinon@10.0.0 latest

☝️ this would get folks unblocked who are broken by 10.0.1, but doesn't require a hard unpublish of 10.0.1.

bcoe added a commit to bcoe/DefinitelyTyped that referenced this issue Apr 9, 2021
https://github.com/sinonjs/fake-timers appears to have added types which are incompatible
with the current implementation in DefinitelyTyped.

Refs: sinonjs/sinon#2353
Refs: sinonjs/sinon#2352
feywind added a commit to feywind/nodejs-pubsub that referenced this issue Apr 9, 2021
feywind added a commit to googleapis/nodejs-pubsub that referenced this issue Apr 9, 2021
* fix: temporarily pin sinon at 10.0.0 until this is fixed:

sinonjs/sinon#2352

* chore: let npm rearrange the imports for some devDependencies
@mroderick
Copy link
Member

But just wanted to make sure you knew about this feature on npm:

npm dist-tag add sinon@10.0.0 latest

☝️ this would get folks unblocked who are broken by 10.0.1, but doesn't require a hard unpublish of 10.0.1.

Top tip! Thank you @bcoe

@mroderick
Copy link
Member

Thank you for the very accurate bug report @kevinoid 👍

Your analysis is correct: the updated version of @sinonjs/fake-timers does indeed introduce a breaking change.

In the npm registry the 10.0.1 package is deprecated and 10.0.0 is marked as latest, so this should not affect any more users (hat tip @bcoe).

Once #2351 is merged and #2353 is resolved, I'll release a new major version detailing the breaking change in the changelog

@bcoe
Copy link

bcoe commented Apr 12, 2021

@mroderick thank you for getting us unblocked 👏

@fatso83
Copy link
Contributor

fatso83 commented Apr 20, 2021

Once #2351 is merged and #2353 is resolved, I'll release a new major version detailing the breaking change in the changelog

@mroderick are we there? I am not sure what is needed to get #2353 resolved. The NPM fix seemed like it resolved the issue and #2351 prevents it, or ...?

@fatso83
Copy link
Contributor

fatso83 commented May 24, 2021

I just retried @regseb's example on the current Sinon code, and I got 0 errors, so it seems like this should be good ... Releasing Sinon 11.

@fatso83
Copy link
Contributor

fatso83 commented May 24, 2021

As Sinon 11 is out, the breaking change comes with a major version and should be good. Mentioned in the changelog.

@fatso83 fatso83 closed this as completed May 24, 2021
@perrin4869
Copy link
Contributor

perrin4869 commented May 25, 2021

My typescript project is failing to compile with sinon@11 with the error mentioned above

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2021

@perrin4869 Make sure you update all the dependencies. Do you still have that error if you clean rm -r node_modules && npm install? I did try out regseb's example locally (using npm link) before publishing, and did not see any issue, and AFAIK the error is caused by the fake-timers dependency, so maybe it's stuck on an earlier revision?

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2021

@perrin4869 If you still have problems, could you please file a new issue and make some reproducible test case for us? I do not have anything to reproduce. The only bit I have works fine.

@perrin4869
Copy link
Contributor

hm... Will try to dig into it later, maybe dependabot did a derp, but looking at the package-lock.json it seems fake-timers updated correctly

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2021

@perrin4869 Remember you also need to update your types. Using the example by @regseb:

$ npm i sinon@11.0.0
+ sinon@11.0.0
updated 6 packages and audited 20 packages in 0.786s

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

$ npx tsc index.js --declaration --allowJs --emitDeclarationOnly
node_modules/@types/sinon/index.d.ts:778:36 - error TS2694: Namespace '"/home/carlerik/dev/nimble/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'TimerId'.

778     type SinonTimerId = FakeTimers.TimerId;
                                       ~~~~~~~

node_modules/@types/sinon/index.d.ts:780:39 - error TS2694: Namespace '"/home/carlerik/dev/nimble/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'InstalledMethods'.

780     type SinonFakeTimers = FakeTimers.InstalledMethods &
                                          ~~~~~~~~~~~~~~~~

node_modules/@types/sinon/index.d.ts:781:20 - error TS2694: Namespace '"/home/carlerik/dev/nimble/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'NodeClock'.

781         FakeTimers.NodeClock &
                       ~~~~~~~~~

node_modules/@types/sinon/index.d.ts:782:20 - error TS2694: Namespace '"/home/carlerik/dev/nimble/node_modules/@sinonjs/fake-timers/types/fake-timers-src"' has no exported member 'BrowserClock'.

782         FakeTimers.BrowserClock & {
                       ~~~~~~~~~~~~


Found 4 errors.


carlerik at FAT-WORKSTATION in ~/dev/nimble (develop)
$ npm i @types/sinon@latest
+ @types/sinon@10.0.0
removed 1 package, updated 1 package and audited 19 packages in 0.455s

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities


carlerik at FAT-WORKSTATION in ~/dev/nimble (develop)
$ npx tsc index.js --declaration --allowJs --emitDeclarationOnly 

# no errors!

@perrin4869
Copy link
Contributor

Ah didn't notice @types/sinon was installed, it wasn't listed in my package.json file. Yeah updating it fixed it, thanks!

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

7 participants