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.

Sign up for GitHub

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

fix: Duplicate declarations #79

Closed
wants to merge 6 commits into from
Closed

fix: Duplicate declarations #79

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cbe0eb1
Fix duplicate declarations
timfish May 23, 2024
52c5ac5
Test for which export takes priority
timfish May 23, 2024
0cda956
Merge remote-tracking branch 'upstream/main' into fix-duplicate-export
timfish May 28, 2024
b0e4e4c
Simplify test since the exports vary by node version
timfish May 28, 2024
f4b3b47
Merge branch 'main' into fix-duplicate-export
timfish May 28, 2024
fcda35a
Exclude duplicate exports
timfish May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ async function processModule ({
// needs to utilize that new name while being initialized from the
// corresponding origin namespace.
const renamedExport = matches[2]
setters.set(`$${renamedExport}${ns}`, `
setters.set(`$${renamedExport}`, `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change invalidate the preceding comment, which is saying:

  1. If a.mjs exports entities from b.mjs
  2. And b.mjs exports a default entity
  3. The default entity exported by b.mjs should be named b under the namespace associated with b.mjs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, need to check that the tests cover this!

Copy link
Contributor Author

@timfish timfish May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does iitm have this namespacing of default exports behaviour? It means that iitm adds additional exports that don't exist without iitm in use. I can see this was added in #53, but why is this desirable?

I assumed we should be exporting exactly the same exports as when the loader is not in use?

> node ./test/hook/v18.19-static-import-gotalike.mjs             

file:///Users/tim/Documents/Repositories/import-in-the-middle/test/hook/v18.19-static-import-gotalike.mjs:22
  defaultClass as DefaultClass,
  ^^^^^^^^^^^^
SyntaxError: The requested module '../fixtures/got-alike.mjs' does not provide an export named 'defaultClass'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:171:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:254:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

Node.js v22.2.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does iitm have this namespacing of default exports behaviour?

IITM basically has to re-implement ESM loading from scratch because it needs to be able to wrap up any of the entities exported by a module. But transitive modules can export entities with the same identifiers as the module that loaded the transitive dependency (or any ancestor module for that matter). This, combined with star exports, means we have to figure out which one is which in some sort of way. The way chosen is a namespace per module.

  1. Support export * from 'module' ESM syntax #43 added support for handling star exports
  2. Fix duplicate default exports #53 attempts to fix the duplicates exports made possible by 1

All of this could be avoided if we had an API where Node does all of the ESM loading and merely gives us back a malleable set of exports.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this could be avoided if we had an API where Node does all of the ESM loading and merely gives us back a malleable set of exports.

Yeah I wish for this too. Is this already being addressed as part of nodejs/node#52219, or do we need to push for a separate proposal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AbhiPrasad yes, that proposal is what we need to participate in to get the API we would like.

let $${renamedExport} = ${ns}.default
export { $${renamedExport} as ${renamedExport} }
set.${renamedExport} = (v) => {
Expand All @@ -191,7 +191,7 @@ async function processModule ({
continue
}

setters.set(`$${n}` + ns, `
setters.set(`$${n}`, `
let $${n} = ${ns}.${n}
export { $${n} as ${n} }
set.${n} = (v) => {
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/duplicate-a.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = 'a'
1 change: 1 addition & 0 deletions test/fixtures/duplicate-b.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = 'b'
2 changes: 2 additions & 0 deletions test/fixtures/duplicate.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './duplicate-a.mjs'
export * from './duplicate-b.mjs'
13 changes: 13 additions & 0 deletions test/hook/duplicate-exports.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Hook from '../../index.js'
import { foo } from '../fixtures/duplicate.mjs'
import { strictEqual } from 'assert'

Hook((exports, name) => {
if (name.endsWith('/duplicate.mjs')) {
// The last export always takes priority
strictEqual(exports.foo, 'b')
exports.foo = '1'
}
})

strictEqual(foo, '1')
Loading