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
52 stars 20 forks source link

fix: cjs resolution when falling back to `parentLoad` #106

Closed timfish closed 1 week ago

timfish commented 3 weeks ago

When falling back to parentLoad on parsing errors (#104), we don't consider CJS.

For CJS, the parentLoad can return { format: 'commonjs', source: undefined }. In this case we should be passing this full result through as the result of load(). Once we start returning a full result from getSource, it becomes redundant and we can have a single load function.

timfish commented 2 weeks ago

While testing this I also found that the logged error was not as helpful as it could be. processModule may parse multiple files in the process of wrapping a module so I've updated it to log specifically which file the parsing failed for.

Error: 'import-in-the-middle' failed to wrap 'file:///Users/tim/Documents/Repositories/import-in-the-middle/test/fixtures/json-attributes.mjs'
    at load (/Users/tim/Documents/Repositories/import-in-the-middle/hook.js:306:21)
    at async nextLoad (node:internal/modules/esm/hooks:750:22)
    at async Hooks.load (node:internal/modules/esm/hooks:383:20)
    at async MessagePort.handleMessage (node:internal/modules/esm/worker:199:18) {
  cause: Error: Failed to parse 'file:///Users/tim/Documents/Repositories/import-in-the-middle/test/fixtures/json-attributes.mjs'
      at getExports (/Users/tim/Documents/Repositories/import-in-the-middle/lib/get-exports.js:115:17)
      at async processModule (/Users/tim/Documents/Repositories/import-in-the-middle/hook.js:131:23)
      at async load (/Users/tim/Documents/Repositories/import-in-the-middle/hook.js:269:25)
      at async nextLoad (node:internal/modules/esm/hooks:750:22)
      at async Hooks.load (node:internal/modules/esm/hooks:383:20)
      at async MessagePort.handleMessage (node:internal/modules/esm/worker:199:18) {
    cause: SyntaxError: Unexpected token (1:40)
        at pp$4.raise (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:3573:15)
        at pp$9.unexpected (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:772:10)
        at pp$9.semicolon (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:749:68)
        at Parser.parseImport (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn-import-attributes/lib/index.js:242:12)
        at pp$8.parseStatement (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:948:51)
        at pp$8.parseTopLevel (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:829:23)
        at Parser.parse (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:601:17)
        at Function.parse (/Users/tim/Documents/Repositories/import-in-the-middle/node_modules/acorn/dist/acorn.js:651:37)
        at getEsmExports (/Users/tim/Documents/Repositories/import-in-the-middle/lib/get-esm-exports.js:37:23)
        at getExports (/Users/tim/Documents/Repositories/import-in-the-middle/lib/get-exports.js:97:14) {
      pos: 40,
      loc: [Position],
      raisedAt: 44
    }
  }
}
timfish commented 2 weeks ago

Is there some test that will show the problem being solved here?

The current reproduction that I have requires @prisma/client which has "engines": {"node": ">=16.13"} so it's not compatible with all the versions tested.

I'll see if I can reduce it down!

timfish commented 2 weeks ago

108 fixes the issue with @prisma/client and I'm unable to find another reproduction that makes these changes necessary. The broken resolution meant that CJS code was trying to load ESM code.

Maybe this PR will be required to support --experimental-require-module?

timfish commented 1 week ago

Closing in favour of #115, #114 and #109