fix: handling of default and star exports

[mohd-akram]

Closes #68, Closes #77, Closes #62, Closes #60

[mohd-akram]

I'm not sure, doesn't #86 ignore the defaultAs passed in? I don't think there's a reason to actually try and parse the export declaration and as I suspected it wasn't used.

[timfish]

It can't rename export default parseInt('1') because it doesn't have a name. It's renamed further up the import graph. and export * from 'x doesn't include default exports.

This PR may well fix it in another way but I'd try copying the tests across the confirm that.

[mohd-akram]

Yes, clearly they do different things so either the test isn't covering it or something else. Not sure tbh. I added the tests here and they pass on my machine.

[mohd-akram]

It turns out there are other problems with the handling of default exports. I reverted the default export changes from #53 as they stemmed from a misunderstanding that export * includes the default export as well. The tests were therefore wrong, so I removed the wrong tests. This likely fixes other cases as well.

[timfish]

I've since opened #87, but it looks from your work that it can be trimmed down even more!

[timfish]

setters as an array won't suffice because when duplicate named exports are found, all of them need to be excluded from export.

[mohd-akram]

It's a runtime error in Node.js if you do that:

import { foo } from './entry.mjs'
         ^^^
SyntaxError: The requested module './entry.mjs' contains conflicting star exports for name 'foo'

where entry.mjs has export * from './file1.js' and export * from './file2.js' where file1.js == file2.js contents.

And if it's in the same file, it's an error as well:

import { foo } from './file1.mjs'
import { foo } from './file2.mjs'

console.log(foo);

import { foo } from './file2.mjs'
         ^^^

SyntaxError: Identifier 'foo' has already been declared

[mohd-akram]

Star exports were also broken. All the tests currently pass on my machine.

[timfish]

It's a runtime error in Node.js if you do that:

import { foo } from './entry.mjs'
         ^^^
SyntaxError: The requested module './entry.mjs' contains conflicting star exports for name 'foo'

Yes its a runtime error in Node, but it should also be a runtime error when using the loader hook. If you export a foo, there will no longer be a runtime error. That means if you find multiple named exports with the same name, they should all be excluded from the exports.

See my addition of a duplicate named exports test here:
https://github.com/DataDog/import-in-the-middle/pull/87/files#diff-a39aaf48c07e70ea14099a7a5597088257a1b5ac55542df3a4d63e63e6546004

That's why I used this to remove duplicates:
https://github.com/DataDog/import-in-the-middle/pull/87/files#diff-e69a24a4c3746fa1ee96a78e12bb12d2dd4eb6e4cacbced2bf1f4084952681d9R119

[mohd-akram]

Adding your test to my PR results in a runtime error as expected:

not ok 7 test/hook/duplicate-exports.mjs
  ---
  stdout: ''
  stderr: >-
    file:///Users/mohamed/Downloads/code/import-in-the-middle/test/fixtures/duplicate.mjs?iitm=true:21
          let $foo = _.foo
              ^

    SyntaxError: Identifier '$foo' has already been declared
        at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
        at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:163:18)
        at callTranslator (node:internal/modules/esm/loader:430:14)
        at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:436:30)

    Node.js v22.2.0

[timfish]

That's not the same runtime error, that's because the emitted iitm code contains duplicate identifiers. You need to exclude foo entirely from the iitm emitted code.

This should not error, but you get the same runtime error as above because of the duplicate identifiers:
import * as lib from './test/fixtures/duplicate.mjs'

[mohd-akram]

Okay, it seems in Node.js there is no runtime error if you do an import *, but if you import {foo} then it's an error. So yeah, this should be handled.

[mohd-akram]

@timfish Added the duplicate handling here as well. I think this PR cleans up more unnecessary code and likely fixes other issues as well (namespaces caused a bug too). What do you think?

[timfish]

Assuming all the tests pass on all node version this simplifies a lot more than my PR #87 so I'd favour this.

As @jsumners-nr says here though, this PR effectively reverts a PR that was supposed to fix something.

I think before we can even consider landing a PR like this with huge changes, we need to improve the tests with some real-world troublesome libraries.

[mohd-akram]

The original PR had tests with it and they still pass, I only removed the tests that were checking for the wrong thing. I agree on needing some more tests. Sentry is a big consumer of this library so they might be able to test this patch too, and more thoroughly.

[timfish]

Looks good!

Rather than me opening another PR to add more tests, do you think it's worth adding this patch to your PR?
import-in-the-middle-0190d24-Add tests.patch

The got test passes on main and this PR fixes the other 3.

@jsumners-nr, do you think this would suffice to confirm got hasn't been broken?

import got, { Options } from 'got'
import { strictEqual } from 'assert'
import Hook from '../../index.js'

Hook((exports, name) => {
  if (name === 'got' && 'Options' in exports) {
    exports.Options = 'nothing'
  }
})

strictEqual(typeof got, 'function')
strictEqual(typeof got.post, 'function')
strictEqual(typeof got.stream, 'function')
strictEqual(typeof got.extend, 'function')

strictEqual(Options, 'nothing')

[jsumners-nr]

newrelic/node-newrelic#1920 is the issue where most of the discoveries come from. So it looks like the following should all work:

import got from 'got'
typeof got.extend === 'function'

import { defineScript } from 'redis'
typeof defineScript === 'function'

import OpenAI from 'openai'
typeof OpenAI === 'function'

[timfish]

@mohd-akram If you update you PR description to include the following, it'll auto-close them when this gets merged:

Closes #68, Closes #77, Closes #62, Closes #60...

[mohd-akram]

@timfish Thanks, updated. There seems to be some problem with the openai.mjs test though, possibly an issue with imhotap. Running imhotap --files test/hook/openai.mjs in Node.js 16 even if it includes nothing but import OpenAI from 'openai' results in a not ok whereas node test/hook/openai.mjs exits with code 0 fine. Shall I remove it for now?

[timfish]

What version of Node does the openai test pass on?

You can limit the test version by prepending a version the test file name:
./test/hook/v18.19-openai.mjs

[mohd-akram]

Thanks, indeed it doesn't work before 18.19.

[mohd-akram]

Had to move the other tests too. Looks good now.

[AbhiPrasad]

Okay seems reasonable enough to me. I tested with some fixtures in https://github.com/nodejs/node/tree/main/test/fixtures/es-modules and tried applying the patch on some sentry internal projects and nothing crashed, let's give it a try!