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
67 stars 24 forks source link

Identifier '$Completions' has already been declared #60

Closed gajus closed 4 months ago

gajus commented 8 months ago

Getting error:

file:///Users/x/Developer/contra/gaia/node_modules/.pnpm/openai@4.20.0/node_modules/openai/resources/index.mjs?iitm=true:55
    let $Completions = namespace.Completions
        ^

SyntaxError: Identifier '$Completions' has already been declared
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:168:18)
    at callTranslator (node:internal/modules/esm/loader:279:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:285:30)

The file contents:

// File generated from our OpenAPI spec by Stainless.
export * from "./chat/index.mjs";
export * from "./shared.mjs";
export { Audio } from "./audio/audio.mjs";
export { Beta } from "./beta/beta.mjs";
export { Completions, } from "./completions.mjs";
export { Embeddings } from "./embeddings.mjs";

After taking a closer look, the problem seems to be that ./chat/index.mjs exports:

// File generated from our OpenAPI spec by Stainless.
export { Chat } from "./chat.mjs";
export { Completions, } from "./completions.mjs";
//# sourceMappingURL=index.mjs.map

which end up conflicting with export { Completions, } from "./completions.mjs";

gajus commented 8 months ago

Got to this error after tackling https://github.com/DataDog/import-in-the-middle/issues/59, for what it is worth.

jsumners-nr commented 8 months ago

https://github.com/DataDog/import-in-the-middle/pull/53 was meant to solve this sort of issue. But we seem to be playing whack-a-mole with the ludicrous number of permutations ESM provides for exporting entities. I am quite open to suggestions here.

bizob2828 commented 5 months ago

It's worth noting this is failing at a higher level now in 4.40.1 of openai. However that release broke cjs exports, and someone logged this issue

npm i openai@4.40.1
node --loader=import-in-the-middle/hook.mjs --input-type=module -e "import OpenAI from 'openai'; const a = new OpenAI();"
    let $OpenAI = $171467968826591576f6c.OpenAI
        ^

SyntaxError: Identifier '$OpenAI' has already been declared
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:167:18)
    at callTranslator (node:internal/modules/esm/loader:285:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:30)
jpintoic commented 4 months ago

I'm trying to setup Sentry for Node.js (type: module) with the following guide https://docs.sentry.io/platforms/javascript/guides/express/install/esm/ wich recommends the --import command line option to import the instrument.js/.mjs file and can't at the moment due to this issue. I keep getting the Identifier '$Completions' has already been declared error. It seems they are using this package in their SDK.

timfish commented 4 months ago

This can be reproduced with:

a.mjs

export const foo = "a"

b.mjs

export const foo = "b"

index.mjs

export * from './a.mjs'
export * from './b.mjs'

But we seem to be playing whack-a-mole with the ludicrous number of permutations ESM provides for exporting entities.

The Map isn't working to de-duplicate because the keys are derived from:

Buffer.from(modFile, 'hex') + Date.now() + randomBytes(4).toString('hex')
image
jpintoic commented 4 months ago

@timfish, looks promising!

jsumners-nr commented 4 months ago

@timfish what is the expected "export" of:

index.mjs

export * from './a.mjs'
export * from './b.mjs'

Genuinely, I do not know what the export should be. Both a.mjs and b.mjs export a single foo symbol. So which one wins, b?

timfish commented 4 months ago

Ok so I just tested Node without the loader and the behaviour depends on the import:

With: a.mjs

export const foo = "a"

b.mjs

export const foo = "b"

duplicate.mjs

export * from './a.mjs'
export * from './b.mjs'

The following:

import * as e from './duplciate.mjs'

console.log('exports', e);

Results in:

exports [Module: null prototype] {  }

And this

import { foo } from './duplciate.mjs'

console.log('foo', foo);

Results in an error

import { foo } from '../fixtures/duplicate.mjs'
         ^^^
SyntaxError: The requested module '../fixtures/duplicate.mjs' contains conflicting star exports for name 'foo'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:131:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:213:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:66:12)

So I guess we need to mirror this behaviour?

jsumners-nr commented 4 months ago

That makes sense for the presented replication. I think we should definitely throw an error in this case since it is clearly an unresolvable situation. However, we need to figure out how to replicate the allowed graph that the OpenAI module seems to be making use of, and determine a way to handle it appropriately.

The whack-a-mole game continues 🤣

timfish commented 4 months ago

Throwing is ok in some circumstances, but we'd rather not have any cases where registering the hook causes code to throw where it didn't before.