dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Early exit when using dd-trace/loader-hook.mjs and cyclic dependencies

Open pwwolf opened this issue 1 year ago • 6 comments

Expected behaviour Node is able to run ES modules with cyclic dependencies without issue. I would expect the same when using the the loader-hook.

Actual behaviour When specifying --loader dd-trace/loader-hook.mjs, the node process exits early with an exit code 13

Steps to reproduce Here are a couple modules with a circular dependency, i.e. a imports from b and b imports from a.

Running node a.mjs works as expected, producing output testB\ntestA and exiting with code 0. Running node --loader dd-trace/loader-hook.mjs a.mjs prints out the standard Custom ESM Loaders warning but then silently exits with code 13 without executing the functions.

a.mjs

import { testB } from './b.mjs';

export function testA() {
        console.log("testA");
}

testB();

b.mjs

import { testA } from './a.mjs';

export function testB() {
        console.log("testB");
        testA();
}

Environment

  • Operation system: MacOS 12.6
  • Node.js version: v18.17.1
  • Tracer version: dd-trace 4.13.1
  • Agent version: n/a
  • Relevant library versions: n/a

pwwolf avatar Aug 29 '23 12:08 pwwolf

I'm also getting this. +1 request for fix :pray:

ChrisBellew avatar Oct 05 '23 08:10 ChrisBellew

Same, please fix 🙏

vecerek avatar Oct 25 '23 18:10 vecerek

@juan-fernandez @rochdev @tlhunter @simon-id

Please merge this or fix the cyclical dependency in the codebase

suhjohn avatar Feb 08 '24 02:02 suhjohn

It looks like a fix was merged in import-in-the-middle ? @tlhunter is this fixed ?

simon-id avatar Feb 12 '24 12:02 simon-id

~~Yeah that's true, this should be working in recent versions of the tracer. @pwwolf can you confirm if this is still an issue?~~

Looks like this is still an issue:

$ node --loader dd-trace/loader-hook.mjs a.mjs
(node:35673) 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("dd-trace/loader-hook.mjs", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
testB
file:///Users/thomas.hunter/Projects/GH-3595/b.mjs:5
                testA();
                ^

ReferenceError: Cannot access 'testA' before initialization
    at testB (file:///Users/thomas.hunter/Projects/GH-3595/b.mjs:5:17)
    at file:///Users/thomas.hunter/Projects/GH-3595/a.mjs:7:1
    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:113:12)

Node.js v20.11.0

tlhunter avatar Feb 12 '24 16:02 tlhunter

I believe that https://github.com/DataDog/import-in-the-middle/pull/36 didn't fully fix the issue.

It looks like it was also reverted (see https://github.com/DataDog/import-in-the-middle/pull/48). I tried a version of the library that supposedly contained the fix and it didn't work though. I verified that exit code 13 was still being returned.

It would be great if this was fixed soon! As more and more packages start only shipping ESM-compatible versions, it's very hard to continue using a commonjs package.

theodormarcu avatar Feb 12 '24 19:02 theodormarcu