Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ESM tests are failing with Node v18.19.0 #1872

Closed
trentm opened this issue Dec 20, 2023 · 3 comments
Closed

ESM tests are failing with Node v18.19.0 #1872

trentm opened this issue Dec 20, 2023 · 3 comments
Assignees
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies

Comments

@trentm
Copy link
Contributor

trentm commented Dec 20, 2023

Many ESM tests using the import-in-the-middle (IITM) hook (i.e.: node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ...) fail when tested with Node v18.19.0 (and Node v20.x) with an error similar to:

 file:///home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-koa/test/fixtures/use-koa.mjs:23
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
         ^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@opentelemetry/instrumentation-http' does not provide an export named 'HttpInstrumentation'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:191:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

Node.js v18.19.0

The issue is as follows. In Node.js v20, Node.js's internal ESM loader changed to do its loading off of the main thread. This change required the import-in-the-middle library to use a different mechanism for determining the set of exports from a CommonJS module being imported. That mechanism has a limitation that it does not support re-exports (exports resulting from export * from './somefile';). nodejs/import-in-the-middle#29

The limitation was not initially an immediate issue for CI in this repo, because currently Node.js v20 is not being tested in CI. Then Node.js backported this ESM off-thread loading to Node.js v18.19.0.

The correct fix is an updated IITM with a fix for that issue. There is a PR (nodejs/import-in-the-middle#30) that hopefully will be considered soon.

temporary workaround

As a temporary workaround, we could:

  1. Pin CI testing of Node v18 to v18.18.2 (the latest release before v18.19.0).
  2. Skip ESM tests for Node.js versions >=18.19.0, and optionally consider testing 18.18.2 and 18.

Given that we hope the import-in-the-middle limitation to be resolved fairly soon in the new year, I favour option 1 for now.

@trentm trentm added the bug Something isn't working label Dec 20, 2023
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Dec 20, 2023
This is as a workaround for ESM testing failing with v18.19.0 due
to node's off-thread ESM loading and import-in-the-middle's limitation
with reexports.

Refs: open-telemetry#1872
pichlermarc pushed a commit that referenced this issue Dec 21, 2023
This is as a workaround for ESM testing failing with v18.19.0 due
to node's off-thread ESM loading and import-in-the-middle's limitation
with reexports.

Refs: #1872
david-luna pushed a commit to david-luna/opentelemetry-js-contrib that referenced this issue Dec 27, 2023
)

This is as a workaround for ESM testing failing with v18.19.0 due
to node's off-thread ESM loading and import-in-the-middle's limitation
with reexports.

Refs: open-telemetry#1872
@pichlermarc
Copy link
Member

Looks like nodejs/import-in-the-middle#30 has merged, we should be able to unpin when the next ittm release is out 🙂

@dyladan dyladan added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies and removed priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization labels Jan 17, 2024
@pichlermarc
Copy link
Member

Looks like DataDog/import-in-the-middle#30 has merged, we should be able to unpin when the next ittm release is out 🙂

For documentation purposes: We've tried unpinning but ran into nodejs/import-in-the-middle#57 which means our ESM checks don't work.
We'll need to either find a workaround or wait for a fix until we can update.

@pichlermarc
Copy link
Member

We're now using iitm@1.7.4, tests are passing again 🙂
Related #2021, #2180

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies
Projects
None yet
Development

No branches or pull requests

3 participants