iambumblehead / esmock

ESM import and globals mocking for unit tests
ISC License
192 stars 18 forks source link

Scoped mocks failing with pnpm but working with npm #302

Open darcyrush opened 3 months ago

darcyrush commented 3 months ago

Ok, this is a was a weird issue to track down, but I have found an issue with either esmock, pnpm/npm or tsimp. I realize that's a lot of options for failure, but I thought I would start here since esmock reliably reproduces the issue.

Basically when mocking a scoped package installed via pnpm, the mock doesn't take effect. When the package is installed via npm the mock takes effect as expected.

Here is a minimal reproduction repository; https://github.com/darcyrush/esmock-scoped-package

# Works
rm -fr node_modules/ && npm i && npm run test

# Fails
rm -fr node_modules/ && pnpm i && npm run test

I haven't tested yarn as I'm not familiar with it, but I imagine it will work as it uses the same install logic as npm. (pnpm uses symlinking of packages)

Node 21.7.3
Ubuntu 22.04 Linux 5.15.0-105-generic x86_64
iambumblehead commented 3 months ago

Thanks for opening the issue. I don't use pnpm myself and the person who added pnpm support hasn't been around lately @koshic

The main file using pnpm resolver is here https://github.com/iambumblehead/esmock/blob/main/src/pnpResolver.js

iambumblehead commented 3 months ago

the demo you made looks great and would probably be a good thing to copy into the esmock tests. It needs this, however,

diff --git a/package.json b/package.json
index bcd50f7..1cc3c44 100644
--- a/package.json
+++ b/package.json
@@ -5,7 +5,7 @@
     "test": "TSIMP_PROJECT=./test/tsconfig.json node --import tsimp/import --test-reporter spec --test 'test/example.test.ts'"
   },
   "dependencies": {
-    "@nestjs/core": "^10.3.8"
+    "@nestjs/core": "^10.3.8",
     "@nestjs/platform-express": "^10.3.8"
   },
   "devDependencies": {

There are many esmock tests, but currently no tests for pnpm specifically

iambumblehead commented 3 months ago

failed test output here looks like this btw

> test
> TSIMP_PROJECT=./test/tsconfig.json node --import tsimp/import --test-reporter spec --test 'test/example.test.ts'

[Nest] 48166  - 2024/05/03 7:56:54     LOG [NestFactory] Starting Nest application...
[Nest] 48166  - 2024/05/03 7:56:54   ERROR [ExceptionHandler] metatype is not a constructor
TypeError: metatype is not a constructor
    at Injector.instantiateClass (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:365:19)
    at callback (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:65:45)
    at async Injector.resolveConstructorParams (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:144:24)
    at async Injector.loadInstance (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:70:13)
    at async Injector.loadProvider (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:97:9)
    at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:56:13
    at async Promise.all (index 0)
    at async InstanceLoader.createInstancesOfProviders (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:55:9)
    at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:40:13
    at async Promise.all (index 1)
✖ test/example.test.ts (2044.283154ms)
  'test failed'
iambumblehead commented 3 months ago

@darcyrush both conditions at this pnpResolver.js line fail

const pnpapi = process.versions.pnp && (await import('pnpapi')).default

process.versions.pnp is undefined and pnpapi is not found when it is imported. Any ideas how to resolve?

koshic commented 3 months ago

@iambumblehead "pnp" (yarn feature, no node_modules on the disk) != "pnpm" (package manager), so undefined is expected.

iambumblehead commented 3 months ago

'nothing to add here, but any thoughts @darcyrush

(I don't know enough about these tools to offer any assistance or input)

iambumblehead commented 3 months ago

adding console.log to esmockModule.js shows the following (differing) paths are resolved for npm vs pnpm

pnpm

{
  moduleId: '../src/example.js',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/src/example.ts'
}
{
  moduleId: '@nestjs/core',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/index.js'
}

npm

{
  moduleId: '../src/example.js',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/src/example.ts'
}
{
  moduleId: '@nestjs/core',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/node_modules/@nestjs/core/index.js'
}

changing the resolver removing realpath conversion; tests still fail with the same error TypeError: metatype is not a constructor

pnpm

{
  moduleId: '../src/example.js',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/src/example.ts'
}
{
  moduleId: '@nestjs/core',
  fileURL: 'file:///home/bumble/software/esmock-scoped-package/node_modules/@nestjs/core/index.js'
}
iambumblehead commented 3 months ago

Per the finding in the previous comment, the error occurs regardless of how esmock resolves the modules so my hunch is that a similar symlink/truepath thing affects tsimp which possibly causes the issue.

darcyrush commented 3 months ago
failed test output here looks like this btw

> test
> TSIMP_PROJECT=./test/tsconfig.json node --import tsimp/import --test-reporter spec --test 'test/example.test.ts'

[Nest] 48166  - 2024/05/03 7:56:54     LOG [NestFactory] Starting Nest application...
[Nest] 48166  - 2024/05/03 7:56:54   ERROR [ExceptionHandler] metatype is not a constructor
TypeError: metatype is not a constructor
    at Injector.instantiateClass (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:365:19)
    at callback (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:65:45)
    at async Injector.resolveConstructorParams (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:144:24)
    at async Injector.loadInstance (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:70:13)
    at async Injector.loadProvider (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/injector.js:97:9)
    at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:56:13
    at async Promise.all (index 0)
    at async InstanceLoader.createInstancesOfProviders (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:55:9)
    at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.2_rxjs@7.8.1/node_modules/@nestjs/core/injector/instance-loader.js:40:13
    at async Promise.all (index 1)
✖ test/example.test.ts (2044.283154ms)
  'test failed'

Yes, this is output for the test failing due to the mock not taking effect. Apologies, I should have stated that in the issue. The test and test case implementation itself is nonsensical, but I couldn't find a more simple name-spaced package to use as a better example.

Thank you for taking the time to look into this. Before opening an issue with tsimp, I wanted to see if other TS runners were affected so I added both tsx and ts-node. It appears they are all impacted by this, which makes me think this may still be related to esmock.

I updated the minimal reproduction repository to help streamline the investigation.