ESM output not functional #3989
Comments
|
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. |
|
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 |
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.
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.
Mostly true but we definitely see users with bundlers in node for one reason or another pretty regularly.
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"
}
} |
Yup, that is a suggestion from https://nodejs.org/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards We'd want to consider whether we want |
|
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; |
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.
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
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.
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 |
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 |
dyladan
added
the
priority:p1
|
Something to add here. I'm attempting to use vite-node in development, and wind up running into the following: If I revert to before the tslib change It seems like there's a good chance this is caused by |
|
@luxaritas you're seeing this error because you're using the ESM output but there's no |
|
NB: I did try adding it, but it complained about (IIRC) the module actually being CJS and not ESM. Either way, it is also broken in this second way |
It must have been something else then, because this field clearly instructs Node.js to treat all .js files as ESM. (unless you edit: now that I think of it, I guess you might be right, requiring that module might have forced it to be loaded as CJS. I'll need to do some experiments, this looks like an interesting case |
|
Currently getting
when trying to run vitest in both Github action CI and locally. Is there a workaround for this right now? |
To me all files in the Regarding my usecase, we're currently writing ESM-only code for Node, which is bundled along with its dependencies with CDK (which uses esbuild). This code is deployed on several lambdas (there are loads of entrypoints, one for each lambda). We're using OpenTelemetry as a direct dependency, primarily for tracing purposes, across all of our packages. Since a small bundle size is paramount to achieve decent cold starts on AWS Lambda, we've started inspecting bundles to find bottlenecks. What we've found is that OpenTelemetry is consistently in the top 5 heaviest dependencies in there, with just What was surprising though is that those modules are loaded from the CJS output, not the ESM one. My guess would be that esbuild detects bad ESM files/config and defaults to the CJS output, but I'm not entirely sure about that process. Using TypeScript's go-to-source on our OpenTelemetry libs' imports also brings us to the CJS output, not the ESM one, so TS also seems to pick up on that. I believe going the ESM-only path would be the easiest solution for OpenTelemetry maintainers in the long run, and it would definitely be the safest bet regarding tree-shaking capabilities. I reckon dual exports of both CJS and ESM can be tricky, since you either deteriorate tree-shaking by reusing CJS outputs from ESM, or you risk having two separate instances between CJS and ESM-using code. If OpenTelemetry really had to support both module systems in the future, I'd personally really appreciate having a "real", dedicated ESM entrypoint that's pure ESM, not just an ESM wrapper around a CJS entrypoint. |
|
I also confirm that the modules are invalid esm, as https://cdn.jsdelivr.net/npm/@opentelemetry/exporter-metrics-otlp-proto@0.45.1/build/esnext/OTLPMetricExporter.js shows there is lack of '.js' extension in imports and this breaks usage. We are currently forcing usage of CJS imports which isn't the best. After a lot of attempts in Effect we decided to not follow the re-export of cjs from esm at all as it breaks compatibility with lots of libraries in the ecosystem, we ended up with isolating global state inside an object embedded in globalThis so that different module instances can access the same state, if you're curious about the discussion you can find it in: Effect-TS/effect#1561 |
|
https://publint.dev/@opentelemetry/resources@1.18.1 This tool detects the issues. |
|
@dyladan Sorry to ping you, but you self-assigned this and marked it as P1. Did you by any chance find some time to spend on this? 🙏 |
|
@klippx I have not unfortunately |
|
Quoting from the original issue description:
With vite 5 it no longer works at all as the missing .js file extensions in the ESM build cause errors. |
|
This should be a relatively easy matter of setting |
|
In theory removing "exports": {
".": {
"browser": {
"import": {
"types": "./build/esm/platform/browser/index.d.ts",
"default": "./build/esm/platform/browser/index.js"
}
},
"node": {
"import": {
"types": "./build/esm/index.d.ts",
"default": "./build/esm/index.js"
},
"require": {
"types": "./build/src/index.d.ts",
"default": "./build/src/index.js"
}
},
"import": {
"types": "./build/esm/index.d.ts",
"default": "./build/esm/index.js"
},
"require": {
"types": "./build/src/index.d.ts",
"default": "./build/src/index.js"
}
}
},The above is also technically a breaking change since it obscures package internals, but that's a simple fix (add a star condition). Edit: This also runs into the issue pointed out previously in the thread where this breaks implicit state between different imports. An end-user application would need to uniformly reference an export or implicit state would break. |
|
If I read the situation correctly @dyladan isn't too interested in dealing with ESM aspect of opentelemetry, hopefully he can 👍 that you take a stab at it and help get it done. The fix SHOULD be simple, but it might not AS simple as you think. Right now if we do this: Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
error: Unexpected token 'export'Since the ESM built artefact is not functional (I guess that's how the title was chosen for this issue). So it's not as simple as fixing a few exports, you also need to ensure valid ESM output ( |
Yah, I think this is the crux of the issue (aside from the shared state issue). TypeScript doesn't natively offer the tools today to support dual packaging. So we'd need to introduce another bundler/packager to at least handle emitting correctly postfixed files. |
Typescript is absolutely capable of outputting valid ESM code, you don't need a bundler for that. You just need to build it with a different tsconfig file. So if you want both ESM and CJS, you just run |
That is exactly what is being done in this repo. I did some additional digging, and the problem with |
The main problem is that there are no |
I wouldn't say I'm not interested, but I have limited time and this hasn't bubbled up the priority queue for me yet. I would indeed be happy for someone to contribute here.
+1 to this. In my experience, double-publishing ESM and commonjs is never as simple as it should be. I'm happy to be proven wrong with a PR in this regard. |
|
While looking at this issue on nodejs I came across a link to this PR on aws-powertools repo that I think we can use as a guide. The PR includes an excellent description with references to various sources and reasonings for changes. There are then multiple follow-up PRs for other packages there, which is probably what we'd need to do as well because there are too many files to include in one PR. For each package we'll need to:
I took a pass at this on the Express instrumentation package because it's much smaller in scope. One thing I'm not sure of is where esnext comes into play here. From other testing, some build tools seem to need it (like vite), but most things I've seen only show CJS and ESM, with ESM actually using ESNext 🤔 |
|
https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext is worth a read. If you use nodenext in the esm type checking it will enforce file extensions etc in the type checker which could help prevent downstream issues. |
|
I think @JamieDanielson summarized pretty accurately what needs to be done and as @JakeGinnivan points out there are some nice things to add to To get even more practical, and to address the concerns of @dyladan, I can share some experience in migrating a package that used to be published to npm only as CJS, to instead get published as both ESM and CJS: To ensure both that the old CJS users are not broken AND ensuring it works as expected for the new target audience (the ESM users) I would recommend a "smoke-test" strategy that I employed which was to set up a monorepo whose only point is to ensure imports are working correctly for different consumer types - from types, to tests, to runtime. And different consumer types being e.g.
All in all a smoke test like this ensures that the types compile, that the tests runs, and finally also that runtime (integration) works for different projects. These are things that can all individually break. This is not an exhaustive test in any sense, it just smoke test to see that we are in the ball park of NOT fundamentally and utterly breaking anyone. The smoke-test consumer project is here https://github.com/klippx/mappersmith-consumer if you want some inspiration, it is not immediately reusable for this project, but can serve as inspo / fast track to get something similar up. |
|
Any workarounds for this issue in the meantime? I'm also getting errors when using opentelemetry with vite-node. @luxaritas did you resolve by pinning to an old version? /node_modules/@opentelemetry/resources/build/esm/platform/node/machine-id/getMachineId-darwin.js:52
import { execAsync } from './execAsync';
^^^^^^
SyntaxError: Cannot use import statement outside a module |
|
@jrestall I'm currently getting it to work by using patch-package to remove the |
|
Excuse me, is |
Same. I add resolve alias in vitest config for temporarily fixing it. {
find: '@opentelemetry/resources',
replacement: 'node_modules/@opentelemetry/resources/build/src/index.js',
} |
@JakeGinnivan Thanks for the link! This was also suggested in #4396 but unfortunately There is a plan to update to TypeScript 5 in the next major SDK version 🤔. I think using @klippx Thanks for confirmation and also for the link to the smoke test project! That looks like an interesting approach worth exploring, and very different from what I originally had in mind when I created this issue to add smoke tests. I'll check out both ideas and see if I can put something together to help improve confidence as we keep going down the path of trying to make ESM fully functional in all expected scenarios. |
|
Yah, I think this might end up be a major-version kinda fix. Possibly three things need to happen:
|
|
@RichiCoder1 luckily we are already working on the next major version in the |
🥳 awesome! I don't know how best I can help, but happy to help! |
What happened?
Steps to Reproduce
Expected Result
being able to compile and run the code
Actual Result
Runtime error:
Notice how it says "is a CommonJS module" even though the package contains an ESM directory. I added
"type": "module"tonode_modules/@opentelemetry/core/package.jsonand I got a different error when compiling: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
The text was updated successfully, but these errors were encountered: