nodejs / import-in-the-middle

Like `require-in-the-middle`, but for ESM import
https://www.npmjs.com/package/import-in-the-middle
Apache License 2.0
72 stars 27 forks source link

Crash when using `import-in-the-middle` with `module.exports = {...require('$dependency')}` #157

Open juan-fernandez opened 1 month ago

juan-fernandez commented 1 month ago

Expected Behavior

Using a myloader.mjs file like this

import * as module from 'module'

module.register('import-in-the-middle/hook.mjs', import.meta.url)

and using NODE_OPTIONS='--import ./myloader.mjs' $my_command does not crash.

Actual Behavior

Running

NODE_OPTIONS='--import ./myloader.mjs' npm run test:vitest

results in

(node:81156) Error: 'import-in-the-middle' failed to wrap 'file:///Users/juan.fernandezdealba/Desktop/dev/starter-kit-juan-fork/node_modules/@prisma/client/default.js'

Steps to Reproduce the Problem

  1. Clone my starter-kit fork:
git clone git@github.com:juan-fernandez/starter-kit.git
cd starter-kit
  1. Install dependencies

    npm i
  2. Run vitest test command with --import flag:

NODE_OPTIONS='--import ./myloader.mjs' npm run test:vitest

See this error:

(node:81156) Error: 'import-in-the-middle' failed to wrap 'file:///Users/juan.fernandezdealba/Desktop/dev/starter-kit-juan-fork/node_modules/@prisma/client/default.js'

Specifications

Running npx envinfo

  System:
    OS: macOS 14.7
    CPU: (10) arm64 Apple M1 Max
  Binaries:
    Node: 20.17.0 - ~/.volta/tools/image/node/20.17.0/bin/node
    npm: 10.8.2 - ~/.volta/tools/image/node/20.17.0/bin/npm

Investigation

If you look at node_modules/@prisma/client/default.js the file looks like this:

module.exports = {
  ...require('.prisma/client/default'),
}

Funnily enough, if you change the file contents to this:

const res = require('.prisma/client/default')
module.exports = {
  ...res,
}

the crash does not happen anymore.

Context

https://github.com/DataDog/dd-trace-js/issues/4713

Extra repro case

https://github.com/juan-fernandez/iitm-issue-repro is lighter. It does not throw the same exact error but seems to be caused with a very similar setup, so it's likely to be related

timfish commented 1 month ago

This doesn't look like a crash. It's a logged error telling you a module could not be wrapped.

juan-fernandez commented 1 month ago

Thanks for the quick response!

no test is run though, meaning vitest crashes somewhere:

➜  starter-kit git:(main) NODE_OPTIONS='--import ./myloader.mjs' npm run test:vitest

> starter-kit@0.0.0 test:vitest
> dotenv -e .env.test vitest run

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

 RUN  v2.0.5 /Users/juan.fernandezdealba/Desktop/dev/starter-kit-juan-fork

(node:5132) Error: 'import-in-the-middle' failed to wrap 'file:///Users/juan.fernandezdealba/Desktop/dev/starter-kit-juan-fork/node_modules/@prisma/client/default.js'
(Use `node --trace-warnings ...` to show where the warning was created)
 ❯ src/server/modules/auth/email/__tests__/email.router.test.ts (0)
(node:5133) Error: 'import-in-the-middle' failed to wrap 'file:///Users/juan.fernandezdealba/Desktop/dev/starter-kit-juan-fork/node_modules/@prisma/client/default.js'
 ❯ src/server/modules/post/__tests__/post.router.test.ts (0)
 ❯ src/server/modules/auth/email/__tests__/email.router.test.ts (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 2 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  src/server/modules/post/__tests__/post.router.test.ts [ src/server/modules/post/__tests__/post.router.test.ts ]
 FAIL  src/server/modules/auth/email/__tests__/email.router.test.ts [ src/server/modules/auth/email/__tests__/email.router.test.ts ]
TypeError: Expected string, array buffer, or typed array to be returned for the "source" from the "load" hook but got undefined.
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯

 Test Files  2 failed (2)
      Tests  no tests
   Start at  14:29:15
   Duration  702ms (transform 20ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 329ms)

vs without the loader:

➜  starter-kit git:(main) npm run test:vitest 

> starter-kit@0.0.0 test:vitest
> dotenv -e .env.test vitest run

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

 RUN  v2.0.5 /Users/juan.fernandezdealba/Desktop/dev/starter-kit

... 
 ❯ src/server/modules/auth/email/__tests__/email.router.test.ts (9)
   ❯ auth.email (9)
 ❯ src/server/modules/auth/email/__tests__/email.router.test.ts (9)
   ❯ auth.email (9)
     ✓ login (4)
     ❯ verifyOtp (5)
 ❯ src/server/modules/auth/email/__tests__/email.router.test.ts (9)
   ❯ auth.email (9)
     ✓ login (4)
     ❯ verifyOtp (5)
       ⠧ should successfully set session on valid OTP
 ✓ src/server/modules/post/__tests__/post.router.test.ts (2)
 ✓ src/server/modules/auth/email/__tests__/email.router.test.ts (9)

 Test Files  2 passed (2)
      Tests  11 passed (11)
   Start at  14:30:17
   Duration  1.36s (transform 97ms, setup 88ms, collect 1.56s, tests 354ms, environment 0ms, prepare 81ms)
juan-fernandez commented 1 month ago

hey @timfish do you need more information? I can also try to debug it myself if you can give me some pointers 😄

timfish commented 1 month ago

I've not had a chance to look yet but my guess is that this is something to do with the parser.. although the parser should only be used for ESM.

juan-fernandez commented 1 month ago

@timfish

although the parser should only be used for ESM.

where's this logic in the code? I'm debugging the issue but AFAICT the parsing happens for every file

jsumners-nr commented 1 month ago

Please make the minimal reproduction a single script.

juan-fernandez commented 1 month ago

@jsumners-nr

The smallest repro case I could create is in

https://github.com/juan-fernandez/iitm-issue-repro

The error is different 🤔, but I think they might be related somehow, and it does look like a bug. Let me know if you have questions 😄

jsumners-nr commented 1 month ago

I am unable to assist. I don't know what require('#dependency') is.

juan-fernandez commented 1 month ago

that's defined in https://github.com/juan-fernandez/iitm-issue-repro/blob/main/package.json#L19-L23

it's a package internal import mimicking the original issue: https://nodejs.org/api/packages.html#imports

karrui commented 2 weeks ago

Waiting on a fix for this issue; this is blocking my org's use of dd-trace or updating of their dependencies.