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
60 stars 22 forks source link

`getExports` used for node v20, v18.19.0 doesn't handle reexports #29

Closed trentm closed 7 months ago

trentm commented 1 year ago

import-in-the-middle hooking of a CommonJS module that has reexports breaks usage of that module. For example:

% node --version
v20.2.0

% cat foo.mjs
import { S3Client, ListBucketsCommand } from '@aws-sdk/client-s3';
console.log('hi');

% node foo.mjs
hi

% node --experimental-loader=import-in-the-middle/hook.mjs foo.mjs
(node:91931) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
file:///Users/trentm/tmp/iitm-node20-exports/foo.mjs:1
import { S3Client, ListBucketsCommand } from '@aws-sdk/client-s3';
                   ^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@aws-sdk/client-s3' does not provide an export named 'ListBucketsCommand'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:122:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:188:5)

Node.js v20.2.0

Steps to Reproduce the Problem

  1. Save this "package.json":
{
  "name": "iitm-node20-exports",
  "version": "1.0.0",
  "license": "MIT",
  "dependencies": {
    "@aws-sdk/client-s3": "^3.363.0",
    "import-in-the-middle": "^1.4.1"
  }
}
  1. Save this "foo.mjs":
import { S3Client, ListBucketsCommand } from '@aws-sdk/client-s3';
console.log('hi');
  1. Run this (using node v20):
node --experimental-loader=import-in-the-middle/hook.mjs foo.js

details

Adding this console.log to import-in-the-middle/lib/get-exports.js:

  if (format === 'commonjs') {
    console.log('XXX IITM getCjsExports of url %s\n-- source --\n%s\n-- parsed --\n%o\n--', url, source, getCjsExports(source))
    return addDefault(getCjsExports(source).exports)
  }
and re-running: ``` % node --experimental-loader=import-in-the-middle/hook.mjs foo.mjs (node:96950) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time (Use `node --trace-warnings ...` to show where the warning was created) XXX IITM getCjsExports of url file:///Users/trentm/tmp/iitm-node20-exports/node_modules/@aws-sdk/client-s3/dist-cjs/index.js -- source -- "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); exports.S3ServiceException = void 0; const tslib_1 = require("tslib"); tslib_1.__exportStar(require("./S3Client"), exports); tslib_1.__exportStar(require("./S3"), exports); tslib_1.__exportStar(require("./commands"), exports); tslib_1.__exportStar(require("./pagination"), exports); tslib_1.__exportStar(require("./waiters"), exports); tslib_1.__exportStar(require("./models"), exports); var S3ServiceException_1 = require("./models/S3ServiceException"); Object.defineProperty(exports, "S3ServiceException", { enumerable: true, get: function () { return S3ServiceException_1.S3ServiceException; } }); -- parsed -- { exports: [ '__esModule', 'S3ServiceException', [length]: 2 ], reexports: [ './S3Client', './S3', './commands', './pagination', './waiters', './models', [length]: 6 ] } -- XXX IITM getCjsExports of url file:///Users/trentm/tmp/iitm-node20-exports/node_modules/@aws-sdk/client-s3/dist-cjs/index.js -- source -- "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); exports.S3ServiceException = void 0; const tslib_1 = require("tslib"); tslib_1.__exportStar(require("./S3Client"), exports); tslib_1.__exportStar(require("./S3"), exports); tslib_1.__exportStar(require("./commands"), exports); tslib_1.__exportStar(require("./pagination"), exports); tslib_1.__exportStar(require("./waiters"), exports); tslib_1.__exportStar(require("./models"), exports); var S3ServiceException_1 = require("./models/S3ServiceException"); Object.defineProperty(exports, "S3ServiceException", { enumerable: true, get: function () { return S3ServiceException_1.S3ServiceException; } }); -- parsed -- { exports: [ '__esModule', 'S3ServiceException', [length]: 2 ], reexports: [ './S3Client', './S3', './commands', './pagination', './waiters', './models', [length]: 6 ] } -- file:///Users/trentm/tmp/iitm-node20-exports/foo.mjs:1 import { S3Client, ListBucketsCommand } from '@aws-sdk/client-s3'; ^^^^^^^^^^^^^^^^^^ SyntaxError: The requested module '@aws-sdk/client-s3' does not provide an export named 'ListBucketsCommand' at ModuleJob._instantiate (node:internal/modules/esm/module_job:122:21) at async ModuleJob.run (node:internal/modules/esm/module_job:188:5) Node.js v20.2.0 ```

That shows the "reexports" I'm referring to.

I was kind of surprised that cjs-module-lexer recognized tslib_1.__exportStar(require("./S3Client"), exports); and similar as a "reexport". Does it have particular smarts about tslib or is it parsing and/or executing tslib?

Does import-in-the-middle want/need to get into recursively handling these "reexports"?

luxaritas commented 1 year ago

In case it's useful, as I've been debugging related issues for my own setup, I took a rough pass at a patch to expose the reexports. This is not my area of expertise so I make no claims that this is the correct way of doing it, but it at least appears to work in my use case.

The following is what I applied (via patch-package - import-in-the-middle+1.4.1.patch) - if it winds up being that this is actually reasonable, I'm happy to work on moving this into a PR. Minimally the way I'm dealing with path resolution here seems... wrong, though

diff --git a/node_modules/import-in-the-middle/lib/get-exports.js b/node_modules/import-in-the-middle/lib/get-exports.js
index cfa86d4..d93516a 100644
--- a/node_modules/import-in-the-middle/lib/get-exports.js
+++ b/node_modules/import-in-the-middle/lib/get-exports.js
@@ -3,12 +3,26 @@
 const getEsmExports = require('./get-esm-exports.js')
 const { parse: getCjsExports } = require('cjs-module-lexer')
 const fs = require('fs')
-const { fileURLToPath } = require('url')
+const { fileURLToPath, pathToFileURL } = require('url')

 function addDefault(arr) {
   return Array.from(new Set(['default', ...arr]))
 }

+async function getFullCjsExports(url, context, parentLoad, source) {
+  const ex = getCjsExports(source)
+  return Array.from(new Set([
+    ...addDefault(ex.exports),
+    ...(await Promise.all(ex.reexports.map(re => getExports(
+       re.startsWith('./') || re.startsWith('../')
+           ? pathToFileURL(require.resolve(fileURLToPath(new URL(re, url)))).toString()
+           : pathToFileURL(require.resolve(re)).toString(),
+       context, 
+       parentLoad
+    )))).flat()
+  ]))
+}
+
 async function getExports (url, context, parentLoad) {
   // `parentLoad` gives us the possibility of getting the source
   // from an upstream loader. This doesn't always work though,
@@ -33,7 +47,7 @@ async function getExports (url, context, parentLoad) {
     return getEsmExports(source)
   }
   if (format === 'commonjs') {
-    return addDefault(getCjsExports(source).exports)
+    return getFullCjsExports(url, context, parentLoad, source)
   }

   // At this point our `format` is either undefined or not known by us. Fall
@@ -44,7 +58,7 @@ async function getExports (url, context, parentLoad) {
     // isn't set at first and yet we have an ESM module with no exports.
     // I couldn't construct an example that would do this, so maybe it's
     // impossible?
-    return addDefault(getCjsExports(source).exports)
+    return getFullCjsExports(url, context, parentLoad, source)
   }
 }
bengl commented 1 year ago

@luxaritas Yes, this seems like a great start. Can you move it to a draft PR, and add tests?

luxaritas commented 1 year ago

Done - let me know if there's anything else I can do to help!

atif-saddique-deel commented 7 months ago

this issue is still happening, is there any fix for it or workaround?