ESM loader in v11 does not work with got package, after installing the latest import-in-the-middle #1920

corevo · 2023-12-20T10:38:22Z

Description

Attempting to use v11 in an ESM project that includes got causes the node process to crash.

In Node v18 you will get a silent exit code 13. In Node v20, it will throw a SyntaxError before exit code 1.

app-1  | (node:8) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
app-1  | --import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("newrelic/esm-loader.mjs", pathToFileURL("./"));'
app-1  | (Use `node --trace-warnings ...` to show where the warning was created)
app-1  | file:///app/node_modules/@payzen/got-commons/node_modules/got/dist/source/index.js?iitm=true:39
app-1  |     let $default = _.default
app-1  |         ^
app-1  |
app-1  | SyntaxError: Identifier '$default' has already been declared
app-1  |     at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:155:18)
app-1  |     at callTranslator (node:internal/modules/esm/loader:285:14)
app-1  |     at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:30)
app-1  |
app-1  | Node.js v20.10.0
app-1 exited with code 1

Expected Behavior

Running node --loader newrelic/esm-loader.mjs should work without crashing node.
NOTE: # ( Tell us what you expected to happen. )

Troubleshooting or NR Diag results

Steps to Reproduce

run npm init -y
run npm i --save got newrelic
create the file index.mjs with the contents

import got from 'got'

run node --loader newrelic/esm-loader.mjs index.mjs
crash

Your Environment

ex: Browser name and version: not relevant
ex: Node version: 20.10.0
ex: Operating System and version: Linux

Additional context

The text was updated successfully, but these errors were encountered:

workato-integration · 2023-12-20T10:38:27Z

https://new-relic.atlassian.net/browse/NR-199684

jsumners-nr · 2023-12-20T12:30:33Z

What version of the agent are you using? What version of import-in-the-middle is being used? The output of npm ls newrelic import-in-the-middle got should help us proceed.

zintus · 2023-12-20T15:36:10Z

zintus/newrelic-node-examples@c209a0d
I believe this repro is related. Please advise

npm run start fails
running node index.js succeeds

Node v21.3.0

corevo · 2023-12-21T09:06:16Z

  └─┬ newrelic@11.7.0
    └── import-in-the-middle@1.7.2

jsumners-nr · 2023-12-21T15:04:27Z

Thank you. I have narrowed down the issue and submitted a PR upstream (nodejs/import-in-the-middle#53).

ljharb · 2023-12-21T22:00:14Z

You can't export default multiple times, so it's possible this is a node loader bug.

jsumners-nr · 2023-12-22T17:03:43Z

Thank you @ljharb. It's a problem with IITM. I wrote a test for core and it passes:

Core Test

diff --git a/test/es-module/test-esm-initialization.mjs b/test/es-module/test-esm-initialization.mjs
index f03a47d5d3..e7882e0839 100644
--- a/test/es-module/test-esm-initialization.mjs
+++ b/test/es-module/test-esm-initialization.mjs
@@ -28,3 +28,19 @@ describe('ESM: ensure initialization happens only once', { concurrency: true },
     assert.strictEqual(code, 0);
   });
 });
+
+describe('ESM: nested default exports are handled correctly', () => {
+  it(async() => {
+    const { code, stderr, stdout } = await spawnPromisified(execPath, [
+      '--experimental-import-meta-resolve',
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'),
+      '--no-warnings',
+      fixtures.path('es-modules', 'nested-default', 'index.mjs'),
+    ]);
+
+    assert.strictEqual(stderr, '');
+    assert.strictEqual(stdout.match(/resolve passthru/g)?.length, 3);
+    assert.strictEqual(code, 0);
+  });
+});
diff --git a/test/fixtures/es-modules/nested-default/a.mjs b/test/fixtures/es-modules/nested-default/a.mjs
new file mode 100644
index 0000000000..dc07e2033d
--- /dev/null
+++ b/test/fixtures/es-modules/nested-default/a.mjs
@@ -0,0 +1,3 @@
+const a = 'a'
+export default a
+export * from './b.mjs'
diff --git a/test/fixtures/es-modules/nested-default/b.mjs b/test/fixtures/es-modules/nested-default/b.mjs
new file mode 100644
index 0000000000..7f5dadf656
--- /dev/null
+++ b/test/fixtures/es-modules/nested-default/b.mjs
@@ -0,0 +1,2 @@
+const b = 'b'
+export default b
diff --git a/test/fixtures/es-modules/nested-default/index.mjs b/test/fixtures/es-modules/nested-default/index.mjs
new file mode 100644
index 0000000000..755d201fc8
--- /dev/null
+++ b/test/fixtures/es-modules/nested-default/index.mjs
@@ -0,0 +1 @@
+export * from './a.mjs'

zintus · 2023-12-23T14:40:16Z

I can confirm your fix helps with my repro too. Thanks for the investigation @jsumners-nr

corevo · 2023-12-24T12:55:58Z

I've tried your fix and the error is now different, it looks like the module loading is okay, but not all the exports are re-exported correctly, it looks like the exported object is got's Request rather than the got object itself, I'm using got@14.0.0 which is a native ESM module.

TypeError: got.extend is not a function
    at file:///app/node_modules/@payzen/got-commons/build/got-commons.js:185:29
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:120:12)

Printing the default exports yields

[class Request extends Duplex]

jsumners-nr · 2023-12-27T12:41:02Z

Thank you for the update. Figuring out the full solution given that information will be tricky indeed.

jsumners-nr · 2023-12-27T15:45:59Z

I have determined the underlying problem. It is going to require some significant work in IITM.

jsumners-nr · 2023-12-28T13:58:27Z

The issue is that in https://github.com/DataDog/import-in-the-middle/blob/ae292fb72d2a3444f3c4fa77439be81df49ff8f9/hook.js#L205-L212 we merge all transitive exports into a singular namespace. Whatever was the last "default" export in derived namespaces ends up being the default exported from the shim module. Which means we need to figure out how to provide all of the required exports from the shim module while also picking the primary default and renaming the others according to their module name (e.g. './foo-bar.js' default export should be renamed and exported as fooBar).

I'm at a loss for how to accomplish this. I welcome any ideas.

corevo · 2023-12-28T15:14:53Z

Sorry but I think I don't fully understand the issue here.

Using the keyword default more than once is illegal, it will return a parsing error of duplicate definition of .default on the module object, are you referring to a module like this

/* first.mjs */
export default 1

/* second.mjs */
export default 2

/* index.mjs */
export * from './first.mjs'
export * from './second.mjs'

Because if so then in index.mjs the .default is not defined because the default keyword was not strictly used and the exported module will be {}, unless you want the default and then you have to rename it if imported from more than one module.

ljharb · 2023-12-28T16:08:37Z

export * from doesn’t bring the default export, so there shouldn’t ever be more than one.

jsumners-nr · 2023-12-28T17:48:01Z

After my last update I had a brainwave and seem to have a viable solution. I'm currently working through cleaning it up and expanding my test to try and cover all bases.

jsumners-nr · 2023-12-28T19:52:48Z

I believe commit nodejs/import-in-the-middle@1abc010 solves the problem.

Ref: newrelic/node-newrelic#1920 <strike>👋 my last "fix" (#43) broke in a new way thanks to the helpful ESM allowance of exporting `default` multiple times. This PR resolves the issue. I suspect there could be further problems with the exports since I have no idea how exporting `default` multiple times is meant to be interpreted, but I suspect any such issues will be an extreme edge case. The only other thing I can think of to do here is to somehow track that we already have a `default` export and munge the names of any subsequent ones.</strike> ----- 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, and only the directly imported `default` gets exported. Still, we have to handle figuring out which `default` is the right one and construct our shim namespace accordingly. ----- 2023-01-03: this can only be supported on Node versions where we parse the modules with an AST parser and interpret the code manually. So I have updated the test suite for this feature to only run on such versions. --------- Co-authored-by: James Sumners <git@zifbang.com>

jsumners-nr · 2024-01-12T13:01:51Z

I think we should see a release soon, but in the meantime, if you'd like to try the latest main branch of import-in-the-middle it should solve the issue.

alexey-yarmosh · 2024-01-12T15:50:00Z

Hey, thanks for the fix. I've just checked with the new version of import-in-the-middle. It fixes got issue.

In the meantime it still fails with another import in our project. When I try to:

import { defineScript } from 'redis';

I get:

node --loader newrelic/esm-loader.mjs test.mjs
(node:97636) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("newrelic/esm-loader.mjs", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
import { defineScript } from 'redis';
         ^^^^^^^^^^^^
SyntaxError: The requested module 'redis' does not provide an export named 'defineScript'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:214:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

Node.js v20.10.0

Also seems problem is specificly with defineScript, cause import { createClient } from 'redis'; works fine.

barryhagan · 2024-01-17T03:45:37Z

In the meantime it still fails with another import in our project. When I try to:
...
Also seems problem is specificly with defineScript, cause import { createClient } from 'redis'; works fine.

This is because of the re-export issue. Only defineScript is a re-export, not createClient.

We've been waiting for some time for this to be able to move to version 11 and it looks like they finally merged the 6 month old PR.

jsumners-nr · 2024-01-18T14:00:04Z

IITM 1.7.3 has been published and resolves the original issue described here. Please update your dependencies to get the fix. If there are other problems, please open a new issue.

alexey-yarmosh · 2024-02-01T09:18:58Z

Hey, do you have plans/approximate dates to update iitm version in node-newrelic itself?

jsumners-nr · 2024-02-01T12:40:26Z

The update is covered by the semver range qualifier specified in the package.json:

node-newrelic/package.json

Line 203 in f414564

"import-in-the-middle": "^1.6.0",

Updating your dependencies will net the fix.

alexey-yarmosh · 2024-02-01T12:42:05Z

Thanks!

zintus · 2024-03-18T17:43:54Z

Thanks for working on this! @jsumners-nr

My repro i've linked before still fails with same steps. Please advise
newrelic/newrelic-node-examples@main...zintus:newrelic-node-examples:main


> esm-app@1.0.0 start
> node --experimental-loader newrelic/esm-loader.mjs -r newrelic index.js

(node:36337) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("newrelic/esm-loader.mjs", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
file:///Users/zintus/workspace/newrelic-node-examples/esm-app/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:167:18)
    at callTranslator (node:internal/modules/esm/loader:285:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:30)

jsumners-nr · 2024-06-24T16:31:29Z

There have been a few new releases of import-in-the-middle. Does your issue still persist, @zintus ?

corevo added bug needs-triage labels Dec 20, 2023

jsumners-nr mentioned this issue Dec 21, 2023

Fix duplicate default exports nodejs/import-in-the-middle#53

Merged

jsumners-nr self-assigned this Dec 21, 2023

jsumners-nr removed the needs-triage label Dec 21, 2023

jsumners-nr closed this as completed Jan 18, 2024

jsumners-nr mentioned this issue May 29, 2024

fix: handling of default and star exports nodejs/import-in-the-middle#85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESM loader in v11 does not work with got package, after installing the latest import-in-the-middle #1920

ESM loader in v11 does not work with got package, after installing the latest import-in-the-middle #1920

corevo commented Dec 20, 2023

workato-integration bot commented Dec 20, 2023

jsumners-nr commented Dec 20, 2023 •

edited

Loading

zintus commented Dec 20, 2023

corevo commented Dec 21, 2023

jsumners-nr commented Dec 21, 2023

ljharb commented Dec 21, 2023

jsumners-nr commented Dec 22, 2023

zintus commented Dec 23, 2023

corevo commented Dec 24, 2023

jsumners-nr commented Dec 27, 2023

jsumners-nr commented Dec 27, 2023

jsumners-nr commented Dec 28, 2023

corevo commented Dec 28, 2023

ljharb commented Dec 28, 2023

jsumners-nr commented Dec 28, 2023

jsumners-nr commented Dec 28, 2023

jsumners-nr commented Jan 12, 2024

alexey-yarmosh commented Jan 12, 2024

barryhagan commented Jan 17, 2024

jsumners-nr commented Jan 18, 2024

alexey-yarmosh commented Feb 1, 2024

jsumners-nr commented Feb 1, 2024

alexey-yarmosh commented Feb 1, 2024

zintus commented Mar 18, 2024

jsumners-nr commented Jun 24, 2024

ESM loader in v11 does not work with got package, after installing the latest import-in-the-middle #1920

ESM loader in v11 does not work with got package, after installing the latest import-in-the-middle #1920

Comments

corevo commented Dec 20, 2023

Description

Expected Behavior

Troubleshooting or NR Diag results

Steps to Reproduce

Your Environment

Additional context

workato-integration bot commented Dec 20, 2023

jsumners-nr commented Dec 20, 2023 • edited Loading

zintus commented Dec 20, 2023

corevo commented Dec 21, 2023

jsumners-nr commented Dec 21, 2023

ljharb commented Dec 21, 2023

jsumners-nr commented Dec 22, 2023

zintus commented Dec 23, 2023

corevo commented Dec 24, 2023

jsumners-nr commented Dec 27, 2023

jsumners-nr commented Dec 27, 2023

jsumners-nr commented Dec 28, 2023

corevo commented Dec 28, 2023

ljharb commented Dec 28, 2023

jsumners-nr commented Dec 28, 2023

jsumners-nr commented Dec 28, 2023

jsumners-nr commented Jan 12, 2024

alexey-yarmosh commented Jan 12, 2024

barryhagan commented Jan 17, 2024

jsumners-nr commented Jan 18, 2024

alexey-yarmosh commented Feb 1, 2024

jsumners-nr commented Feb 1, 2024

alexey-yarmosh commented Feb 1, 2024

zintus commented Mar 18, 2024

jsumners-nr commented Jun 24, 2024

jsumners-nr commented Dec 20, 2023 •

edited

Loading