ESM output not functional

[paulius-valiunas]

What happened?

Steps to Reproduce

import { W3CBaggagePropagator } from "@opentelemetry/core"

Expected Result

being able to compile and run the code

Actual Result

Runtime error:

SyntaxError: Named export 'W3CBaggagePropagator' not found. The requested module '@opentelemetry/core' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@opentelemetry/core';
const { W3CBaggagePropagator } = pkg;

Notice how it says "is a CommonJS module" even though the package contains an ESM directory. I added "type": "module" to node_modules/@opentelemetry/core/package.json and I got a different error when compiling:

error TS2305: Module '"@opentelemetry/core"' has no exported member 'W3CBaggagePropagator'.

Additional Details

The ESM output is broken and doesn't follow ESM spec. First of all, the package.json does not contain "type": "module" so ES Modules will never be loaded from this package. And even if I add this line, the code inside ./build/esm is broken anyway.

For example, @opentelemetry/core/build/esm/index.d.ts has lines such as:

export * from './baggage/propagation/W3CBaggagePropagator';

This will not work in ESM because ESM requires the file extension to be in the import path. The line should be:

export * from './baggage/propagation/W3CBaggagePropagator.js';

I suggest you either fix the ESM output to make it actually functional, or remove it altogether. Because right now it just falls back to CJS. Maybe it works in its current state with some exotic bundlers, but it certainly doesn't follow the ESM spec.

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

No response

[dyladan]

The ESM output comes from typescript and works in at least rollup and webpack, and is primarily for web browser bundling. For direct node use we are a commonjs module at least for now. We are willing to revisit that decision if there is interest and benefit.

For now it seems like the issue is actually not that the ESM is broken but that the CommonJS fails to load when loaded from an ESM application. Also a major issue, but a little bit different.

[paulius-valiunas]

I would say the thing you're doing with CJS is more of a workaround than a proper solution. Also, I'm very surprised this works with webpack, because it's not standard ESM and it all looks like one big hack.

These days everyone's migrating to ESM in Node.js so you will have to drop CJS at some point. That would also have the benefit of reducing the size of node_modules, since bundlers are not normally used with Node.js. I just think that fixing what you currently have (it's as easy as adding .js to your imports and "type": "module" to your package.json) would be a good step towards doing that in the future. Instead of making all Node.js projects rely on your CJS output, it would then only be a fallback if the runtime doesn't support ESM.

[dyladan]

I would say the thing you're doing with CJS is more of a workaround than a proper solution.

Can you explain what you mean by this? The module is a regular CommonJS module. We're not working around anything here. Any workarounds or nonstandard practice are related to ESM not CJS.

These days everyone's migrating to ESM in Node.js so you will have to drop CJS at some point.

Maybe true but if we publish as an ES module, we can't be used from CJS projects without code changes. Something we're not willing to do until at least the next major version revision.

That would also have the benefit of reducing the size of node_modules, since bundlers are not normally used with Node.js.

Mostly true but we definitely see users with bundlers in node for one reason or another pretty regularly.

I just think that fixing what you currently have (it's as easy as adding .js to your imports and "type": "module" to your package.json) would be a good step towards doing that in the future. Instead of making all Node.js projects rely on your CJS output, it would then only be a fallback if the runtime doesn't support ESM.

I think we may be able to do something like this:

// package.json
{
  "main": "./build/src/index.js",
  "exports": {
    "import": "./build/esm/index.js",
    "require": "./build/src/index.js",
    "default": "./build/src/index.js",
  },
  "type": "module"
}

// tsconfig.esm.json
{
  "extends": "./tsconfig.base.es5.json",
  "compilerOptions": {
    "module": "ES6",
    "moduleResolution": "node16"
  }
}

[trentm]

I think we may be able to do something like this:

Yup, that is a suggestion from https://nodejs.org/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards
Note the differences in "Approach # 1" and "Approach # 2" discussed there.

We'd want to consider whether we want {"exports": { "import": ... to point to a small ESM wrapper -- what the Node.js docs call "Approach # 1". Otherwise, it would be possible to have separate copies of the library loaded (one from CJS, one from ESM). Perhaps the separately loaded copies wouldn't be a concern (instanceof checks failing, separate state).

[klippx]

I agree with @paulius-valiunas this isn't working and I came to this repo only to say the same thing.

What we want to write:

import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';

What we have to write to make it work:

import * as SemanticConventionsModule from '@opentelemetry/semantic-conventions';
// Do not ask me what they are doing here
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const HackedSemanticConventionsModule = (SemanticConventionsModule as any)
  .default as typeof SemanticConventionsModule;
const { SemanticResourceAttributes } = HackedSemanticConventionsModule;

[paulius-valiunas]

Can you explain what you mean by this? The module is a regular CommonJS module. We're not working around anything here. Any workarounds or nonstandard practice are related to ESM not CJS.

I mean the package is not a pure CJS package, you do have ESM modules in this package but they can't be used. If they can't be used, then why do you have them there in the first place? It doubles the package size for no reason.

Maybe true but if we publish as an ES module, we can't be used from CJS projects without code changes. Something we're not willing to do until at least the next major version revision.

It can. I'm not asking you to remove the CJS modules, they can continue working the exact same way as they are now. If someone uses require with this package, it will ignore the ESM directory the same way it does now, nothing will change in that regard. All I'm asking for is a working import("@opentelemetry/core") without relying on Node.js' backwards compatibility layer. I know it would be a far fetch if you only currently had CJS modules in here, but you already have a hybrid CJS/ESM package, it just needs a little touch up to work properly with all runtimes and bundlers.

Mostly true but we definitely see users with bundlers in node for one reason or another pretty regularly.

Well I didn't say bundlers are never used in Node.js, it's just not the most standard approach. I was trying to say the majority of Node.js users don't use bundlers. And that means removing CJS would benefit most (but not all) Node.js users.

I think we may be able to do something like this:

// package.json
{
  "main": "./build/src/index.js",
  "exports": {
    "import": "./build/esm/index.js",
    "require": "./build/src/index.js",
    "default": "./build/src/index.js",
  },
  "type": "module"
}

// tsconfig.esm.json
{
  "extends": "./tsconfig.base.es5.json",
  "compilerOptions": {
    "module": "ES6",
    "moduleResolution": "node16"
  }
}

There are many ways to do this, I think your example is fine, although it has a quirky side effect of not allowing people to require a specific module (require("@opentelemetry/core/build/src/something")) - because that module will be interpreted as CJS. The solution would be to add a ./build/src/package.json that contained only a "type": "commonjs" line. There are more detailed solutions here: https://2ality.com/2019/10/hybrid-npm-packages.html

[paulius-valiunas]

I think we may be able to do something like this:

Yup, that is a suggestion from nodejs.org/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards Note the differences in "Approach # 1" and "Approach # 2" discussed there.

We'd want to consider whether we want {"exports": { "import": ... to point to a small ESM wrapper -- what the Node.js docs call "Approach # 1". Otherwise, it would be possible to have separate copies of the library loaded (one from CJS, one from ESM). Perhaps the separately loaded copies wouldn't be a concern (instanceof checks failing, separate state).

That's a very good point, I didn't initially think about all the global/static state you have in these packages. For example, different modules call trace.getActiveSpan(), but they get different instances of trace - will they get the same span? Will one of them get an empty span or no span at all?... I'm not very well versed in the architecture of these packages but it sounds like it could potentially be a problem. But then I guess that's the same as people trying to use different versions of opentelemetry in their packages (does that currently work? I haven't tested...)

[luxaritas]

Something to add here. I'm attempting to use vite-node in development, and wind up running into the following:

 /app/node_modules/@opentelemetry/resources/build/esm/platform/node/machine-id/getMachineId-linux.js:1
 import { __awaiter, __generator, __values } from "tslib";
 ^^^^^^
 
 SyntaxError: Cannot use import statement outside a module
     at internalCompileFunction (node:internal/vm:73:18)
     at wrapSafe (node:internal/modules/cjs/loader:1176:20)
     at Module._compile (node:internal/modules/cjs/loader:1218:27)
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
     at Module.load (node:internal/modules/cjs/loader:1117:32)
     at Function.Module._load (node:internal/modules/cjs/loader:958:12)
     at Module.require (node:internal/modules/cjs/loader:1141:19)
     at require (node:internal/modules/cjs/helpers:110:18)
    at /app/node_modules/@opentelemetry/resources/src/platform/node/machine-id/getMachineId.ts:25:25
     at ViteNodeRunner.directRequest (file:///app/node_modules/vite-node/dist/client.mjs:341:5)

Node.js v18.16.0

If I revert to before the tslib change

 /app/node_modules/@opentelemetry/resources/build/esm/platform/node/machine-id/getMachineId-linux.js:63
 import { promises as fs } from 'fs';
 ^^^^^^
 
 SyntaxError: Cannot use import statement outside a module
     at internalCompileFunction (node:internal/vm:73:18)
     at wrapSafe (node:internal/modules/cjs/loader:1176:20)
     at Module._compile (node:internal/modules/cjs/loader:1218:27)
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
     at Module.load (node:internal/modules/cjs/loader:1117:32)
     at Function.Module._load (node:internal/modules/cjs/loader:958:12)
     at Module.require (node:internal/modules/cjs/loader:1141:19)
     at require (node:internal/modules/cjs/helpers:110:18)
     at /app/node_modules/@opentelemetry/resources/src/platform/node/machine-id/getMachineId.ts:25:25
    at ViteNodeRunner.directRequest (file:///app/node_modules/vite-node/dist/client.mjs:341:5)

Node.js v18.16.0

It seems like there's a good chance this is caused by node_modules/@opentelemetry/resources/build/esm/platform/node/machine-id/getMachineId.js using require to import that file which uses import. import()ing a CJS file is fine, but going the other way around causes issues. :)

[paulius-valiunas]

@luxaritas you're seeing this error because you're using the ESM output but there's no "type": "module" in its package.json. It doesn't matter if you use require or import to import the getMachineId-linux.js module, this will always happen because the module itself is broken. This is exactly the problem I was talking about: the ESM output of these packages is incomplete and cannot be used in Node.js.