Cannot redefine property error when using jest.spyOn after v1.2.206

[ldiqual]

Describe the bug

SWC version v1.2.206 is causing our jest setup file to error out when calling jest.spyOn. v1.2.205 does not error out.

Input code

// jest.setup.ts
import * as cache from './packages/foo/src/common/cache'
const redis = new RedisMock(7481)
beforeEach(() => {
  jest.spyOn(cache, 'getRedis').mockResolvedValue(redis)
})

// cache.ts
import memoize from 'p-memoize'
import { getConfig } from './config'
export const getRedis = memoize(async () => {
  const config = await getConfig()
  return new Redis(config.redisUrl)
})

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "decorators": true
    },
    "target": "es2021",
    "keepClassNames": true
  }
}

Playground link

No response

Expected behavior

jest.spyOn should stub cache.getRedis

Actual behavior

jest.spyOn fails with the following error:

TypeError: Cannot redefine property: getRedis
    at Function.defineProperty (<anonymous>)
    at ModuleMocker.spyOn (/home/circleci/project/node_modules/jest-mock/build/index.js:820:16)
    at Object.spyOn (/home/circleci/project/jest.setup.ts:26:8)
    at Promise.then.completed (/home/circleci/project/node_modules/jest-circus/build/utils.js:333:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/home/circleci/project/node_modules/jest-circus/build/utils.js:259:10)
    at _callCircusHook (/home/circleci/project/node_modules/jest-circus/build/run.js:240:40)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at _runTest (/home/circleci/project/node_modules/jest-circus/build/run.js:202:5)

Version

v1.2.206+

Additional context

No response

[Austaras]

Duplicate of #3843?

[ldiqual]

I don't believe it is. This is an issue introduced specifically by v1.2.206 from a couple days ago, while the issue you're referencing dates back to 2021 and doesn't seem to actually throw an error.

[kdy1]

No it's duplicate.
We improved module pass recently.

Aligning to ES specificarion is always a bugfux

[kdy1]

If you think this is not a duplicate, please provide a repro which use ESM

[ldiqual]

@kdy1 thanks for looking into this. I'm trying to understand what the issue is and how to resolve it.

I believe #3843 is about stubbing an exported function that is called within its module. This is not what I'm doing here. Instead, I'm trying to stub an exported function that is called outside of its module, and it errors out on the jest.spyOn line, which I believe should be possible in commonjs. I do agree that it shouldn't be possible with ESM.

Our codebase is typescript transpiling to commonjs and we don't use ESM. This is why I don't understand your comment about aligning to ES specification, since this is a commonjs output.

Are you saying that SWC no longer supports stubbing functions exported by commonjs modules, as of v1.2.206, and should not going forward?

[kdy1]

It's basically the same thing, and #3843 is not the only one about wrong usage of exports.
There were 7 (8 including this) issues and there was an issue for this case too

[kdy1]

If you want cjs behavior, you can use common js instead of ESM imports, or use import foo = require('foo') instead

[magic-akari]

import * as cache from './packages/foo/src/common/cache'
jest.spyOn(cache, 'getRedis').mockResolvedValue(redis)

In ESM mode, you can never write anything on the exports from a module.
the spyOn method is trying to rewrite getRedis method which is forbid in esm mode.

[magic-akari]

@kdy1

If you want cjs behavior, you can use common js instead of ESM imports, or use import foo = require('foo') instead

Unfortunately, import foo = require('foo') will not fix this issue.
The key is how to export a module.
If the module is exported from an ESM module, you should not have a way to modify it directly from the outside.

In CJS world, you cannot import an ESM module, you will never run into this issue.

If I remember correctly, node can natively interoperate between ESM and CJS .
I will check the node interop behaviour.

[kdy1]

If the module is exported from an ESM module, you should not have a way to modify it directly from the outside.

Oh, I see., thanks!

[ldiqual]

Just to clarify again: none of these modules (either jest.setup.ts or cache.ts) is ESM. They are typescript files that are transpiled to commonjs modules. I set them up so that they compile to commonjs, and I expect import * as cache from './cache' to transpile to a require, and to require a commonjs module.

Is the new behavior to transpile a module to ESM if it is required/imported using import? Would explicitly setting module.type to commonjs in swcrc fix this?

[magic-akari]

@ldiqual If you do not expect an esm behaviour, do not use esm syntax. use CJS module.exports, or CTS export synatx.

function getRedis() { }
export = {
    getRedis,
    foo,
    bar
}

[shahmirn]

Running into the same thing. Worked fine in v1.2.204

After upgrading and getting the error, I tried downgrading and still ran into the same issue in my local machine / environment, so for some reason, it's not downgrading correctly either.

[kdy1]

@shahmirn This is not a bug.
If you don't want the behavior of ESM, you should not use ESM because ESM has a specification about the behavior of imports and exports.

[shahmirn]

@kdy1 I agree that the new behavior may not be a bug, but rather is fixing a previous bug, and the change results in a stricter / more correct adherence to the esm specification, but an argument can be made that this is a breaking change, and hence warrants a major version bump.

Furthermore, as @ldiqual said, I'm also using typescript, and our tsconfig.json has module set to CommonJS, module resolution to node, and target as es2020, so I'm not sure how esm is even coming into the picture.

[kdy1]

Nope, I don't think breaking code depending on wrong behavior is a breaking change. It's a bugfix.

About ESM part, you used ESM syntax

[magic-akari]

When I set module: commonjs what does it mean?
Do I get CJS or ESM?
Both, and neither.

It is CJS, because the output code has exports/module.exports.
It is not CJS, because it is transformed from ESM.
Users are using ESM syntax, such as export default, export { foo }, etc.
Users expect ESM behaviour, such as export is live-binding, export cannot be overwritten.
These are ESM features. We simulate ESM behaviour in CJS.

If the user expects CJS, it is a misuse to use these syntaxes.
There is no equivalent export syntax in ESM.
However, the exports.foo = expr syntax still works, as will TS's export = expr.

But what if the user really wants ESM and wants to test in jest?
This is jest's problem, which is equivalent to how jest is used in an ESM environment.

[magic-akari]

• Meta: Native support for ES Modules jestjs/jest#9430
• jest.mock does not mock an ES module without Babel jestjs/jest#10025

[marchaos]

@shahmirn - downgrading requires you to clear your jest cache. swc-jest does not create a cache key using the version of swc/core, and therefore the cache doesn't invalidate between version upgrades / downgrades.

[a88zach]

If this is truly a bugfix and not a bug, you should update the intro text here: https://swc.rs/docs/usage/jest

This is no longer a drop in replacement for ts-jest as reverting to ts-jest reverts to the previous behavior

[kdy1]

@a88zach tsc will be fixed to disallow this wrong behavior. I'm not sure when it will be, though

[kdy1]

Okay. I decided not to change the documentation because it's a still drop-in replacement if you don't do wrong thing

[loomchild]

One workaround that I've found it so use ts-jest to transform just the module to be mocked (./packages/foo/src/common/cache in your case), and @swc/jest for the rest (in jest.config.js):

transform: {
  '^.+packages/foo/src/common/cache\\.ts$': 'ts-jest',
  '^.+\\.ts$': '@swc/jest'
}

It'll a bit slower, but if there aren't many tests requiring spying functionality, it might be acceptable.

[MakingStuffs]

Is this nuance/issue being left as it is or being worked on? I know there is some debate as to whether it should actually be ts-jest that are fixing their implementation but in the meantime it would be good for everyone looking to switch to SWC to have a solid workaround for this issue.

If it is not being worked on I think we need to remove the line about @swc/jest being a drop-in replacement for ts-jest as it simply isn't. The reasoning (who has the correct ESM implementation) is beside the point as just dropping in @swc/jest will 100% break a lot of working tests that are implementing spyOn in ts-jest and the current workaround seems to require that users place modules in separate files -- requires a lot of reworking for existing apps.

[kdy1]

I don't think breaking an already-broken code is wrong
The problem is not @swc/jest here. The problem is the wrong input.

[kdy1]

This issue will never be worked on, as a compiler should not fix wrong code

[MakingStuffs]

@kdy1 thanks for clarifying I just wanted to check so I knew as, philosophical debates aside, @swc/jest cannot be considered a drop-in replacement for ts-jest as there is a lot of reworking required in order to ensure currently passing tests continue to pass after swapping out the packages.

Other than putting pretty much every dependant function into its own module and then mocking it, do you have any other solutions/recommendations regarding this issue?

[kdy1]

I think @swc/jest is still a drop-in replacement for ts-jest as it does not break the correct code.

See #5205

[kdy1]

I locked comments because #5151 is the correct thread to discuss about this