Sinon stubbing failed after upgrade to 3.9.2

[zorji]

TypeScript Version: 3.9.2, 3.9.3

Search Terms: sinon, stub

Code

// file: ./src/stubber/index.ts
export { toStub } from './to-stub'

// file: ./src/stubber/to-stub.ts
export const toStub = () => {
  console.log('real stub')
}

// file: ./src/start.ts
import * as sinon from 'sinon'
import * as stubber from './stubber'
(async () => {
  sinon.stub(stubber, 'toStub').callsFake(() => {
    console.log('fake stub')
  })
  stubber.toStub()
})()

Run the start.ts script by compiling it with tsc or simply use ts-node.
I'd expect it print fake stub but now it prints real stub. It's working as expected for [email protected] or lower, start breaking from 3.9.2.

Expected behavior:

Print "fake stub"

Actual behavior:

Print "real stub"

Playground Link: Can't provide Playground link because it contains multiple files

Related Issues: Nope

[nico1510]

The same problem exists for jasmine's spys. I tracked the problem down to this change #32264. After this change get and set properties of modules imported with star syntax are no longer enumerable which then causes a crash in jasmine's check here.
Most likely the same can be said for Sinon and I also saw a comment from someone having this problem with jest here.

Should we tell those libraries to adapt their code or can we have a flag to keep those properties enumerable?

[msholty-fd]

I found this code to break when upgrading from 3.8.2 to 3.9.2. We use ts-jest to transform our TypeScript Jest tests.

import * as foo from 'some/path'
jest.spyOn(foo, 'bar');

I would get an error like:

<spyOn> : bar is not declared writable or has no setter

I resolved this by doing this workaround:

jest.mock('some/path', () => ({
  ...jest.requireActual('some/path'),
}));
const foo = require('some/path');

It's not pretty, but it at least unblocked me from upgrading.

[RyanCavanaugh]

ES module exports are specified to not be enumerable, so this is the expected behavior. Prior behavior was out of compliance with the spec.

[mdurling]

Does your comment still apply if we are using "commonjs" modules, @RyanCavanaugh? I noticed the following difference between the emitted modules:

Before

seconds: [Function: seconds],
minutes: [Function: minutes],
hours: [Function: hours],
days: [Function: days]

After

days: [Getter],
hours: [Getter],
minutes: [Getter],
seconds: [Getter]

Both modules work fine, but the latter (3.9) breaks our ability to mock the imported functions with Sinon. We've had to roll back to 3.8 until we can resolve.

[RyanCavanaugh]

If you're targeting commonjs but writing ES module imports/exports, TS is still going to try to give you ES module behavior.

This code is effectively not legal ES, since it's attempting to modify (indirectly via sinon.stub) a readonly property (the properties of something imported with import * as name are not mutable). It should never have worked.

[typescript-bot]

This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

[squidfunk]

See jasmine/jasmine#1817 (comment) for a temporary workaround.

[adjerbetian]

@RyanCavanaugh Sorry, I didn't understand your answer when you said

If you're targeting commonjs but writing ES module imports/exports

I created a demo project here typescript-3.9-commonjs with a very simple node script.
Do you mean that I should write index.ts differently?

[JordanPawlett]

I found this code to break when upgrading from 3.8.2 to 3.9.2. We use ts-jest to transform our TypeScript Jest tests.

import * as foo from 'some/path'
jest.spyOn(foo, 'bar');

I would get an error like:

<spyOn> : bar is not declared writable or has no setter

I resolved this by doing this workaround:

jest.mock('some/path', () => ({
  ...jest.requireActual('some/path'),
}));
const foo = require('some/path');

It's not pretty, but it at least unblocked me from upgrading.

Just the workaround i was looking for!

[denieler]

Yeah, having the same issue, very annoying 👍

[ahnpnl]

Hi @SimenB not sure if you are aware of this ? I assume once jest supports fully esm this won’t be an issue ?

[SimenB]

Jest module mocks should work regardless, but you probably need to use import() so it's not evaluated statically.

That said, semantics in Jest are probably off for ESM behaviour up until ESM support stabilises

[AndyOGo]

@RyanCavanaugh
You are right, ES module exports shall not be enumerable.

Though may I ask you to consider testability.
And please do not publish breaking changes as a patch.

[squidfunk]

@AndyOGo see jasmine/jasmine#1817 (comment) for an easy workaround monkey-patching tslib. I'm using this hack across 40 packages (Node.js and browser), works perfectly. This should work with any testing library supporting import mocking.

[AndyOGo]

Related:
microsoft/tslib#103

[FauxFaux]

I did a similar thing with babel, https://github.com/snyk/babel-plugin-mutable-imports , but ended up just forking sinon to make it warn, and fixing all the code. sinonjs/sinon#2319

[shawnjones253]

I noticed this only breaks if there is an export x from y in between your test code and the module you want to stub. If the module/function you want to stub is directly exported already, you can directly import it in your test code and this still works. Ended up saving me some refactoring and going down the proxyquire rabbit hole.

// file: ./src/stubber/index.ts
export { toStub } from './to-stub'

// file: ./src/stubber/to-stub.ts
export function toStub() {
  console.log('real stub')
}

// file: ./src/start.ts
import * as sinon from 'sinon'
import * as stubber from './stubber'
import * as toStub from './stubber/to-stub'

(async () => {
  // breaks: logs 'real stub' because toStub is an intermediate reference here
  // (imported via the 'export from' in 'stubber/index')
  sinon.stub(stubber, 'toStub').callsFake(() => {
    console.log('fake stub')
  })
  stubber.toStub()

  // works: because toStub is a direct reference
  // (imported via 'export function' in 'stubber/to-stub')
  sinon.stub(toStub, 'toStub').callsFake(() => {
    console.log('fake stub')
  })
  toStub.toStub()
})()

[cqcmdwym]

If you're targeting commonjs but writing ES module imports/exports, TS is still going to try to give you ES module behavior.

This code is effectively not legal ES, since it's attempting to modify (indirectly via sinon.stub) a readonly property (the properties of something imported with import * as name are not mutable). It should never have worked.

Hi @RyanCavanaugh so if we are targeting 'commonjs', how can be writing without ES module imports/exports in ts 3.9? I mean how can we make it mutable? Thanks.
i mean "module": "commonjs",
"target": "es2017",