Scoped mocks failing with pnpm but working with npm #302
Comments
|
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 |
|
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 |
|
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/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/injector.js:365:19)
at callback (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/injector.js:65:45)
at async Injector.resolveConstructorParams (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/injector.js:144:24)
at async Injector.loadInstance (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/injector.js:70:13)
at async Injector.loadProvider (/home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/injector.js:97:9)
at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/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/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/instance-loader.js:55:9)
at async /home/bumble/software/esmock-scoped-package/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/injector/instance-loader.js:40:13
at async Promise.all (index 1)
✖ test/example.test.ts (2044.283154ms)
'test failed' |
|
@darcyrush both conditions at this pnpResolver.js line fail const pnpapi = process.versions.pnp && (await import('pnpapi')).default
|
|
@iambumblehead "pnp" (yarn feature, no node_modules on the disk) != "pnpm" (package manager), so undefined is expected. |
|
'nothing to add here, but any thoughts @darcyrush (I don't know enough about these tools to offer any assistance or input) |
|
adding console.log to 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/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/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 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'
} |
|
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. |
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 I updated the minimal reproduction repository to help streamline the investigation. |
|
Hey all 👋 Did anyone find a fix for this by any chance? Just hit this myself... |
|
It would be great if esmock would handle these pnpm situations. I haven't used pnpm and so there has not been an organic reason for me personally to resoilve this. esmock wraps the growing complexities of esm module resolution, loader behaviour, etc. and so, even for myself, fixing this issue is likely difficult. |
|
installing pnpm means downloading a binary and giving it access to your system (not nice) |
|
edit removed aggression (I apologize, I almost got hit by a car today and I misdirected my anger here) I understand your concerns about giving pnpm binary access to your system. However, it's worth noting that many of us already give similar access to tools like node and npm because they help us accomplish important tasks. It's natural to be cautious, but I believe it's important to try something before forming strong opinions about it. I write this in defense of pnpm. In a competition between npm vs. pnpm, pnpm wins in almost every metric. Install times, cache size, monorepo support, etc. |
|
I think this resolves the issue diff --git a/src/esmockLoader.js b/src/esmockLoader.js
index 0b16977..51c3f11 100644
--- a/src/esmockLoader.js
+++ b/src/esmockLoader.js
@@ -25,9 +25,13 @@ const isnotfoundRe = /isfound=false/
const iscommonjsmoduleRe = /^(commonjs|module)$/
const isstrict3 = /strict=3/
const hashbangRe = /^(#![^\n]*\n)/
+
+const moduleIdEsc = str =>
+ str.indexOf('+') >= 0 ? str.replace(/(?!\\)\+/g, '\\+') : str
+
// returned regexp will match embedded moduleid w/ treeid
const moduleIdReCreate = (moduleid, treeid) => new RegExp(
- `.*(${moduleid}(\\?${treeid}(?:(?!#-#).)*)).*`)
+ `.*(${moduleIdEsc(moduleid)}(\\?${treeid}(?:(?!#-#).)*)).*`)
// node v12.0-v18.x, global
const mockKeys = global.mockKeys = (global.mockKeys || {})@insanity54 thanks for chiming in, my apology for not being more careful with my message. |
|
planning to copy @darcyrush's test case into the esmock pipeline before merging the PR and publishing |
|
I won't be around to finish this for at least a few hours... |
|
planning to merge and publish soon |
|
anyone participating or reading this thread, please report back and share your experience using pnpm with esmock version 2.6.8+ |
Ok, this is a was a weird issue to track down, but I have found an issue with either
esmock,pnpm/npmortsimp. I realize that's a lot of options for failure, but I thought I would start here sinceesmockreliably reproduces the issue.Basically when mocking a scoped package installed via
pnpm, the mock doesn't take effect. When the package is installed vianpmthe mock takes effect as expected.Here is a minimal reproduction repository; https://github.com/darcyrush/esmock-scoped-package
I haven't tested
yarnas I'm not familiar with it, but I imagine it will work as it uses the same install logic asnpm. (pnpmuses symlinking of packages)The text was updated successfully, but these errors were encountered: