Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

renamedExport handling incorrectly adds ES module exports #68

Closed
trentm opened this issue Mar 29, 2024 · 2 comments · Fixed by #85
Closed

renamedExport handling incorrectly adds ES module exports #68

trentm opened this issue Mar 29, 2024 · 2 comments · Fixed by #85

Comments

@trentm
Copy link
Contributor

trentm commented Mar 29, 2024

The renamedExport handling from #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
Copy link
Contributor Author

trentm commented Apr 1, 2024

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

@jsumners-nr
Copy link
Contributor

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.

@bengl bengl closed this as completed in #85 May 31, 2024
bengl pushed a commit that referenced this issue May 31, 2024
Closes #68, Closes #77, Closes #62, Closes #60

Co-authored-by: Tim Fish <tim@timfish.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants