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

`renamedExport` handling incorrectly adds ES module exports #68

Closed trentm closed 1 month ago

trentm commented 3 months ago

The renamedExport handling from https://github.com/DataDog/import-in-the-middle/pull/53 incorrectly adds ES module exports. Given these three small test files:

% cat default-and-star-a.mjs
export default function funcA() {
  console.log('hi from a');
}
export const valueA = 'a';

% cat default-and-star-b.mjs
export * from './default-and-star-a.mjs';
export const valueB = 'b';

% cat default-and-star-main.mjs
import Hook from './index.js'
Hook((exports, name, baseDir) => {
  if (!name.includes('default-and-star')) return;
  console.log('Hooked name=%s exports:', name, exports)
})

import * as mod from './default-and-star-b.mjs'
console.log('default-and-star-b mod is:', mod)

When run without IITM, we expect the import of './default-and-star-b.mjs' to only have the valueA and valueB exports:

% node default-and-star-main.mjs
default-and-star-b mod is: [Module: null prototype] { valueA: 'a', valueB: 'b' }

However, when running with IITM (at tip of current "main") a renamedExport for the export * from './default-and-star-a.mjs'; statement in "default-and-star-b.mjs" is incorrectly added:

% node --no-warnings --experimental-loader=./hook.mjs default-and-star-main.mjs
Hooked name=/Users/trentm/tm/import-in-the-middle6/default-and-star-a.mjs exports: { default: [Function: funcA], valueA: 'a' }
Hooked name=/Users/trentm/tm/import-in-the-middle6/default-and-star-b.mjs exports: { valueA: 'a', valueB: 'b', defaultAndStarA: [Function: funcA] }
default-and-star-b mod is: [Module: null prototype] {
  defaultAndStarA: [Function: funcA],
  valueA: 'a',
  valueB: 'b'
}
Hooked name=/Users/trentm/tm/import-in-the-middle6/default-and-star-main.mjs exports: {}

Was this possibly a misunderstanding of import behaviour while working on #53? The #53 description says:

I have since learned that ESM doesn't actually allow exporting default multiple times. Transitive default exports get mapped to some other name for the module that has imported them [...]

Where did the impression that "default exports get mapped to some other name for the module that has imported them" come from? Or perhaps I'm not following what the is saying.

Also the change to "hook.js" includes this comment:

      // When we encounter modules that re-export all identifiers from other
      // modules, it is possible that the transitive modules export a default
      // identifier. Due to us having to merge all transitive modules into a
      // single common namespace, we need to recognize these default exports
      // and remap them to a name based on the module name. [...]

Is this possibly a misunderstanding as well? Taking the "default-and-star-b.mjs" example from above:

export * from './default-and-star-a.mjs';
export const valueB = 'b';

That export * ... statement does not re-export the default export from "default-and-star-a.mjs". Therefore, there should not be any need for import-in-the-middle to be adding some normalization of that property name to the hooked namespace or have a setter for it.


Assuming we agree this is a bug that should be fixed (i.e. that this was a misunderstanding), I'll have a draft PR soonish to fix this, along with some simplifications in getSource and processModule.

trentm commented 3 months ago

I forgot to /cc @jsumners-nr and @bengl when I opened this. Please see above. Thanks.

jsumners-nr commented 3 months ago

I'd need to review the comments I left throughout. But the fact is, we are basically re-implementing the parsing and tree building of ESM, and there are way too many permutations to consider in that spec.

If my code is wrong and you know how to make it right: please make it so.