Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Named esm import fails on v1.15.0 #3976

Closed
anajavi opened this issue Jul 7, 2023 · 45 comments · Fixed by #4011 or #4016
Closed

Named esm import fails on v1.15.0 #3976

anajavi opened this issue Jul 7, 2023 · 45 comments · Fixed by #4011 or #4016
Assignees
Labels
bug Something isn't working has:reproducer This bug/feature has a minimal reproducer provided priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc

Comments

@anajavi
Copy link

anajavi commented Jul 7, 2023

What happened?

Steps to Reproduce

test.mjs:

import { envDetectorSync } from '@opentelemetry/resources';

Expected Result

In version 1.14.0 the import works.

Actual Result

In version 1.15.0 the import produces:

import { envDetectorSync } from '@opentelemetry/resources';
         ^^^^^^^^^^^^^^^
SyntaxError: Named export 'envDetectorSync' not found. The requested module '@opentelemetry/resources' 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/resources';
const { envDetectorSync } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Node.js v18.16.1

Additional Details

Same is happening with in version 0.41.0:

import { OTLPMetricExporter } from '@opentelemetry/exporter-metrics-otlp-proto';

OpenTelemetry Setup Code

// test.mjs

import { envDetectorSync } from '@opentelemetry/resources';
import { OTLPMetricExporter } from '@opentelemetry/exporter-metrics-otlp-proto';

package.json

"dependencies": {
    "@opentelemetry/api": "^1.4.1",
    "@opentelemetry/exporter-metrics-otlp-proto": "^0.41.0",
    "@opentelemetry/resources": "^1.15.0",
    "@opentelemetry/sdk-metrics": "^1.15.0",
  }

Relevant log output

No response

@anajavi anajavi added bug Something isn't working triage labels Jul 7, 2023
@applied-mathematician
Copy link

Similar sudden break for me
SyntaxError: Named export 'ConsoleSpanExporter' not found. The requested module '@opentelemetry/sdk-trace-base' is a CommonJS module, which may not support all module.exports as named exports.

dbcfd added a commit to ceramicnetwork/observability that referenced this issue Jul 10, 2023
Per open-telemetry/opentelemetry-js#3976, 1.15.0 and
0.41.0 versions have import failures. This commit pins those dependencies.

Additionally, test is setup correctly so that observability does not require
jest as a dependency.
@MichalZalecki
Copy link

Right now the path that points to esm build is defined under the "module" field which isn't officially supported by Node (it was a proposal and it's still supported by some bundlers). To tell Node the package supports ESM and let it import files from ./build/esm, package.jsons from opentelemetry packages have to have the following fields:

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

https://nodejs.org/api/packages.html#conditional-exports

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

Are you using import-in-the-middle?

@dyladan dyladan self-assigned this Jul 12, 2023
@dyladan dyladan added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc and removed triage labels Jul 12, 2023
@pichlermarc
Copy link
Member

pichlermarc commented Jul 12, 2023

@dyladan created a reproducer here https://github.com/pichlermarc/repro-3976
Edit: I'm using Node v18.12.1

@JamieDanielson
Copy link
Member

I am curious if the tslib package is part of this? tslib was added recently in #3914 . I am seeing some issues on the tslib repo that reference problems with ESM e.g. microsoft/tslib#180. I think there should be fixes for ESM support included in version 2.5.3 if we upgrade to that (it's currently at 2.3.1)

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

I wouldn't expect tslib to affect other dependencies since it doesn't add a loader or anything that should modify non-tslib imports

@pichlermarc pichlermarc added the has:reproducer This bug/feature has a minimal reproducer provided label Jul 12, 2023
@pichlermarc
Copy link
Member

pichlermarc commented Jul 12, 2023

In the reproducer above, it (tslib) also resolves to 2.6.0 🤔

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

The diff between 1.14.0 and 1.15.0 package.json doesn't seem to show anything that I think would cause this:

3c3
<   "version": "1.14.0",
---
>   "version": "1.15.0",
30c30
<     "precompile": "lerna run version --scope $(npm pkg get name) --include-dependencies",
---
>     "precompile": "cross-var lerna run version --scope $npm_package_name --include-dependencies",
66c66
<     "@types/mocha": "10.0.0",
---
>     "@types/mocha": "10.0.1",
68c68
<     "@types/sinon": "10.0.13",
---
>     "@types/sinon": "10.0.15",
71c71,72
<     "karma": "6.3.16",
---
>     "cross-var": "1.1.0",
>     "karma": "6.4.2",
76c77
<     "karma-spec-reporter": "0.0.32",
---
>     "karma-spec-reporter": "0.0.36",
78,79c79,81
<     "mocha": "10.0.0",
<     "nock": "13.0.11",
---
>     "lerna": "7.1.1",
>     "mocha": "10.2.0",
>     "nock": "13.3.1",
81c83
<     "sinon": "15.0.0",
---
>     "sinon": "15.1.2",
85,86c87,88
<     "webpack-cli": "4.9.1",
<     "webpack-merge": "5.8.0"
---
>     "webpack-cli": "4.10.0",
>     "webpack-merge": "5.9.0"
92,93c94,96
<     "@opentelemetry/core": "1.14.0",
<     "@opentelemetry/semantic-conventions": "1.14.0"
---
>     "@opentelemetry/core": "1.15.0",
>     "@opentelemetry/semantic-conventions": "1.15.0",
>     "tslib": "^2.3.1"
97c100
<   "gitHead": "edebbcc757535bc88f01340409dbbecc0bb6ccf8"
---
>   "gitHead": "06e919d6c909e8cc8e28b6624d9843f401d9b059"

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

I believe it is the change in how tslib is re-exporting in the index.js files. I think it is surprising the compatibilty code in Node.js core that loads a CommonJS module as an ESM module.

"node_modules/@opentelemetry/resources/build/src/index.js" before (in v1.4.0):

var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __exportStar = (this && this.__exportStar) || function(m, exports) {
    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
};
Object.defineProperty(exports, "__esModule", { value: true });
__exportStar(require("./Resource"), exports);
__exportStar(require("./platform"), exports);
__exportStar(require("./types"), exports);
__exportStar(require("./config"), exports);
__exportStar(require("./detectors"), exports);

and after:

Object.defineProperty(exports, "__esModule", { value: true });
const tslib_1 = require("tslib");
(0, tslib_1.__exportStar)(require("./Resource"), exports);
(0, tslib_1.__exportStar)(require("./IResource"), exports);
(0, tslib_1.__exportStar)(require("./platform"), exports);
(0, tslib_1.__exportStar)(require("./types"), exports);
(0, tslib_1.__exportStar)(require("./config"), exports);
(0, tslib_1.__exportStar)(require("./detectors"), exports);
(0, tslib_1.__exportStar)(require("./detect-resources"), exports);

If I manually tweak an install of @opentelemetry/resources@1.5.0 to restore the old way of handling re-exports just for the detectors, then that .mjs ESM named import works.

Here is my manual diff
% diff -ur node_modules/@opentelemetry/resources.orig node_modules/@opentelemetry/resources
diff -ur node_modules/@opentelemetry/resources.orig/build/src/detectors/index.js node_modules/@opentelemetry/resources/build/src/detectors/index.js
--- node_modules/@opentelemetry/resources.orig/build/src/detectors/index.js	2023-07-12 09:50:36
+++ node_modules/@opentelemetry/resources/build/src/detectors/index.js	2023-07-12 09:45:30
@@ -14,10 +14,24 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
+    if (k2 === undefined) k2 = k;
+    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
+}) : (function(o, m, k, k2) {
+    if (k2 === undefined) k2 = k;
+    o[k2] = m[k];
+}));
+var __exportStar = (this && this.__exportStar) || function(m, exports) {
+    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
+};
+
+
 Object.defineProperty(exports, "__esModule", { value: true });
 const tslib_1 = require("tslib");
 (0, tslib_1.__exportStar)(require("./BrowserDetector"), exports);
-(0, tslib_1.__exportStar)(require("./EnvDetector"), exports);
+// XXX
+// (0, tslib_1.__exportStar)(require("./EnvDetector"), exports);
+__exportStar(require("./EnvDetector"), exports);
 (0, tslib_1.__exportStar)(require("./BrowserDetectorSync"), exports);
 (0, tslib_1.__exportStar)(require("./EnvDetectorSync"), exports);
 //# sourceMappingURL=index.js.map
diff -ur node_modules/@opentelemetry/resources.orig/build/src/index.js node_modules/@opentelemetry/resources/build/src/index.js
--- node_modules/@opentelemetry/resources.orig/build/src/index.js	2023-07-12 09:50:25
+++ node_modules/@opentelemetry/resources/build/src/index.js	2023-07-12 09:44:45
@@ -14,6 +14,18 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
+    if (k2 === undefined) k2 = k;
+    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
+}) : (function(o, m, k, k2) {
+    if (k2 === undefined) k2 = k;
+    o[k2] = m[k];
+}));
+var __exportStar = (this && this.__exportStar) || function(m, exports) {
+    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
+};
+
+
 Object.defineProperty(exports, "__esModule", { value: true });
 const tslib_1 = require("tslib");
 (0, tslib_1.__exportStar)(require("./Resource"), exports);
@@ -21,6 +33,7 @@
 (0, tslib_1.__exportStar)(require("./platform"), exports);
 (0, tslib_1.__exportStar)(require("./types"), exports);
 (0, tslib_1.__exportStar)(require("./config"), exports);
-(0, tslib_1.__exportStar)(require("./detectors"), exports);
+// (0, tslib_1.__exportStar)(require("./detectors"), exports);
+__exportStar(require("./detectors"), exports);
 (0, tslib_1.__exportStar)(require("./detect-resources"), exports);
 //# sourceMappingURL=index.js.map

(Note that I switched from envDetectorSync to envDetector for my test, just because envDetectorSync wasn't exported in version 1.4.0.) Here is it working:

% cat test.mjs
import { envDetector } from '@opentelemetry/resources';

% node test.mjs

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

The actual outputted JS files differ in the expected tslib manner:

diff {14,15}/build/esm/Resource.js
16,78c16
< var __assign = (this && this.__assign) || function () {
<     __assign = Object.assign || function(t) {
<         for (var s, i = 1, n = arguments.length; i < n; i++) {
<             s = arguments[i];
<             for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p))
<                 t[p] = s[p];
<         }
<         return t;
<     };
<     return __assign.apply(this, arguments);
< };
< var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
<     function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
<     return new (P || (P = Promise))(function (resolve, reject) {
<         function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
<         function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
<         function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
<         step((generator = generator.apply(thisArg, _arguments || [])).next());
<     });
< };
< var __generator = (this && this.__generator) || function (thisArg, body) {
<     var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
<     return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
<     function verb(n) { return function (v) { return step([n, v]); }; }
<     function step(op) {
<         if (f) throw new TypeError("Generator is already executing.");
<         while (_) try {
<             if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
<             if (y = 0, t) op = [op[0] & 2, t.value];
<             switch (op[0]) {
<                 case 0: case 1: t = op; break;
<                 case 4: _.label++; return { value: op[1], done: false };
<                 case 5: _.label++; y = op[1]; op = [0]; continue;
<                 case 7: op = _.ops.pop(); _.trys.pop(); continue;
<                 default:
<                     if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
<                     if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
<                     if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
<                     if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
<                     if (t[2]) _.ops.pop();
<                     _.trys.pop(); continue;
<             }
<             op = body.call(thisArg, _);
<         } catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
<         if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
<     }
< };
< var __read = (this && this.__read) || function (o, n) {
<     var m = typeof Symbol === "function" && o[Symbol.iterator];
<     if (!m) return o;
<     var i = m.call(o), r, ar = [], e;
<     try {
<         while ((n === void 0 || n-- > 0) && !(r = i.next()).done) ar.push(r.value);
<     }
<     catch (error) { e = { error: error }; }
<     finally {
<         try {
<             if (r && !r.done && (m = i["return"])) m.call(i);
<         }
<         finally { if (e) throw e.error; }
<     }
<     return ar;
< };
---
> import { __assign, __awaiter, __generator, __read } from "tslib";

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

From https://nodejs.org/api/esm.html#interoperability-with-commonjs (emphasis mine)

When importing CommonJS modules [with import ...], the module.exports object is provided as the default export. Named exports may be available, provided by static analysis as a convenience for better ecosystem compatibility.

IIUC, that static analysis isn't perfect. I think this tslib change has tickled a case where that analysis doesn't pick up the named exports.

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

@trentm interesting. That means the node module loader is actually loading build/src/index and not build/esm/index as I would have expected.

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

@dyladan Yes, that is my understanding. That correlates with the exception message: "The requested module '@opentelemetry/resources' is a CommonJS module".

"@opentelemetry/resources/package.json" doesn't have a "type" field, so Node.js' loader defaults to considering it a CommonJS module. I'm not super confident that that is the complete logic Node.js' loader uses, however. If I'm right, then I don't know if the "build/esm" and "build/esnext" dirs are ever used.

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

They are used by bundlers like webpack

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

https://nodejs.org/api/packages.html#packagejson-and-file-extensions

Within a package, the package.json "type" field defines how Node.js should interpret .js files. If a package.json file does not have a "type" field, .js files are treated as CommonJS.

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

Also just above that in https://nodejs.org/api/packages.html#packagejson-and-file-extensions

There is the ECMAScript module loader:

...
It can be used to load JavaScript CommonJS modules. Such modules are passed through the cjs-module-lexer to try to identify named exports, which are available if they can be determined through static analysis.

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

That cjs-module-lexer module can be used separately to see what the Node.js core is getting in that static analysis.
Here is a small script to use it:

% npm install cjs-module-lexer

% cat lex-attempt.js
var fs = require('fs');
var { parse } = require('cjs-module-lexer');
var path = process.argv[2]
var content = fs.readFileSync(path, 'utf8')
var result = parse(content)
console.log(result)

If we look at @opentelemetry/resources@1.5.0, it doesn't pick up on the reexports:

% npm ls @opentelemetry/resources
c@1.0.0 /Users/trentm/tmp/c
└── @opentelemetry/resources@1.15.0

% node lex-attempt.js node_modules/@opentelemetry/resources/build/src/index.js
{ exports: [ '__esModule' ], reexports: [] }

but if we look at an install of v1.4.0 (which I have in my ~/tmp/d dir), the lexer does pick up on the reexports:

% (cd ~/tmp/d; npm ls @opentelemetry/resources)
d@1.0.0 /Users/trentm/tmp/d
└── @opentelemetry/resources@1.4.0

% node lex-attempt.js ~/tmp/d/node_modules/@opentelemetry/resources/build/src/index.js
{
  exports: [ '__esModule' ],
  reexports: [ './Resource', './platform', './types', './config', './detectors' ]
}

So I'm pretty confident this is the breakage. I'm not sure what to do about it.

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

If I reduce my diff to just this:

% diff -ur resources.orig node_modules/@opentelemetry/resources
diff -ur resources.orig/build/src/detectors/index.js node_modules/@opentelemetry/resources/build/src/detectors/index.js
--- resources.orig/build/src/detectors/index.js	2023-07-12 10:26:54
+++ node_modules/@opentelemetry/resources/build/src/detectors/index.js	2023-07-12 10:27:39
@@ -17,7 +17,7 @@
 Object.defineProperty(exports, "__esModule", { value: true });
 const tslib_1 = require("tslib");
 (0, tslib_1.__exportStar)(require("./BrowserDetector"), exports);
-(0, tslib_1.__exportStar)(require("./EnvDetector"), exports);
+tslib_1.__exportStar(require("./EnvDetector"), exports);
 (0, tslib_1.__exportStar)(require("./BrowserDetectorSync"), exports);
 (0, tslib_1.__exportStar)(require("./EnvDetectorSync"), exports);
 //# sourceMappingURL=index.js.map
diff -ur resources.orig/build/src/index.js node_modules/@opentelemetry/resources/build/src/index.js
--- resources.orig/build/src/index.js	2023-07-12 10:26:24
+++ node_modules/@opentelemetry/resources/build/src/index.js	2023-07-12 10:25:37
@@ -21,6 +21,6 @@
 (0, tslib_1.__exportStar)(require("./platform"), exports);
 (0, tslib_1.__exportStar)(require("./types"), exports);
 (0, tslib_1.__exportStar)(require("./config"), exports);
-(0, tslib_1.__exportStar)(require("./detectors"), exports);
+tslib_1.__exportStar(require("./detectors"), exports);
 (0, tslib_1.__exportStar)(require("./detect-resources"), exports);
 //# sourceMappingURL=index.js.map

I.e. just remove the (0, tslib_1.__exportStar) technique. Then it works:

% node lex-attempt.js node_modules/@opentelemetry/resources/build/src/index.js
{ exports: [ '__esModule' ], reexports: [ './detectors' ] }

% node test.mjs

I don't know if TypeScript compilation can be configured to not use the (0, tslib_1.__exportStar) technique. I also don't know what functionality is lost by not using that technique; I forget what utility it provides.

@trentm
Copy link
Contributor

trentm commented Jul 12, 2023

I also don't know what functionality is lost by not using that technique; I forget what utility it provides.

Ah, it is to cut the binding to this -- https://stackoverflow.com/a/46397327/14444044

AFAIK, this isn't used in tslib.__exportStar so it shouldn't make a difference, but I'm not very confident in that.

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

Thanks @trentm for doing the legwork. It seems to me like the quickest way to get this resolved is to roll back the tslib change #3914 and do a patch release until we can determine if this side-effect can be mitigated or a long term fix is found. I suspect our use of main, module, imports etc in packge.json is to blame here.

Maybe we should consider publishing only an ECMAScript module and removing commonjs entirely from our bundle? Node 12 (our minimum node version) and later supports ES modules.

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

/cc @MSNev

@dyladan
Copy link
Member

dyladan commented Jul 12, 2023

Another quick fix might be to set "type": "commonjs"

This does not fix the issue

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

can you make a recommendation as to what we should do as a short term fix and what might be a good long term solution?

Oof. I agree that reverting the tslib change is the quickest "fix". I don't know the tslib change well and the improvements it provided to know how painful that would be (e.g. for @MSNev).

There should probably be an added test that does some spot checks that named imports from an ESM import works.

My main struggle is that I can't think of any good long term solutions -- or haven't yet.

@MSNev
Copy link
Contributor

MSNev commented Jul 13, 2023

I don't know the tslib change well

tslib is just the helper functions that TypeScript uses exported as single helper functions, without this TypeScript "adds" the helper functions inline into the generated code.

the improvements it provided to know how painful that would be

And the "improvements" are then that a single version of the function is referenced and used rather than a unique version for every usage where TypeScript would normally emit the helper.
eg.

import { __spreadArray } from "tslib"`

function x(){
_spreadArray(blah);
}

vs inlined

function __spreadArray() {
// The current implementation (based on the TypeScript version)
}
// And then the usage as above
function x(){
_spreadArray(blah);
}

So when bundling there will only every be a single "referenced" version of the helper function (like __spreadArray) when using tslib, and when not EVERY usage of things like the spread operator (...) causes the helper to be emitted into the JS code and then when bundled EVERY function is treated as a different function (even though they are the same) -- bloating the size of the code.

@MSNev
Copy link
Contributor

MSNev commented Jul 13, 2023

There should probably be an added test that does some spot checks that named imports from an ESM import works.

👍

My main struggle is that I can't think of any good long term solutions -- or haven't yet.

We NEED (for bundle size) to include and use tslib, if the issue is the __exportStar helper then this can be "fixed" by banning (and changing) the usage of export * as blah from "some package"; -- This can be a build check -- not sure if there is already a lint check for this.

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

There should probably be an added test that does some spot checks that named imports from an ESM import works.

I think this is tslib's test for the equivalent: https://github.com/microsoft/tslib/blob/main/test/validateModuleExportsMatchCommonJS/index.js

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

While I was writing this, @MSNev you again mentioned export * and I finally connected that you are probably saying what I say in "don't use export *" below. If so, then this may be the fastest fix. I didn't initially understand your meaning yesterday in #3976 (comment)

Some possible solutions (I haven't tried these out).

exports.import + ESM wrapper

A small ESM export wrapper around the CommonJS module could be written, and
then use "exports.import" in "package.json" to point to it. This was mentioned
at #3989 (comment)

This is basically "Approach # 1" from https://nodejs.org/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards

I believe tslib itself provides an example of this.
The ESM wrapper is here: https://github.com/microsoft/tslib/blob/main/modules/index.js
Here is their package.json "exports" definition: https://github.com/microsoft/tslib/blob/2.6.0/package.json#L29-L46

Note that there is a lot going on in that block, much that I don't grok. The recent tslib git history shows a lot of changes to that block, which may indicate that best practices here are still unclear. Originally tslib's "export" block was just:

    "exports": {
        ".": {
            "import": "./modules/index.js",
            "default": "./tslib.js"
        },
        "./": "./"
    }

Needing a separate ESM wrapper might be a maintenance burden:

  1. for top-level exports, and
  2. if additional entry points are defined, as being discussed in Improve API import entrypoints #3908

exports.import + fixed build/esm

Add the following, or a variation:

    "exports": {
        "import": "./build/esm/index.js",
        "default": "./build/src/index.js"
    }

and fix "build/esm/..." to work. (Currently it apparently works with Webpack and other bundlers, but the emitted JS import statements don't include file extensions, which is required. See https://nodejs.org/api/esm.html#mandatory-file-extensions)

This is basically "Approach # 2" from https://nodejs.org/api/packages.html#writing-dual-packages-while-avoiding-or-minimizing-hazards along with the potential pitfall of the package being loaded twice (once as CJS and once as ESM).

don't use export *

I.e. do this as a starter patch:

diff --git a/packages/opentelemetry-resources/src/index.ts b/packages/opentelemetry-resources/src/index.ts
index 3f66901f..a198a9d0 100644
--- a/packages/opentelemetry-resources/src/index.ts
+++ b/packages/opentelemetry-resources/src/index.ts
@@ -14,8 +14,8 @@
  * limitations under the License.
  */

-export * from './Resource';
-export * from './IResource';
+export { Resource } from './Resource';
+export { IResource } from './IResource';
 export * from './platform';
 export * from './types';
 export * from './config';

I think this means that tslib.__exportStar() won't get used and we won't hit the issue.
(I haven't tried this, so I'm not sure if it works.)

@MSNev
Copy link
Contributor

MSNev commented Jul 13, 2023

If tslib usage gains enough popularity,

tslib has been around forever, and for the web it is one of the primary mechanism for reducing the bundle size, so I would say that it ALREADY has enough popularity...

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

so I would say that it ALREADY has enough popularity...

Cool. I'm just totally ignorant around a lot of the TS world. :)

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

@MSNev Perhaps this is silly question. Why would web users of OTel modules be using the CommonJS in build/src/... instead of the ESM in build/esm/... for their bundled JS builds? Unless they are supporting IE and Opera Mini, or (very possibly) I don't know how to read https://caniuse.com/mdn-javascript_operators_import

@MSNev
Copy link
Contributor

MSNev commented Jul 13, 2023

It depends on several things which include their packaging system, the targeted environment (as you called out) and their code they are creating.

For example you can create modules (ESM's) using packaging systems that include CJS packages -- TypeScript and the packaging system takes care of the heavy lifting for you...

It "really" only affects people that are using / consuming the JS packages directly (via a CDN, manual load or not using any form of bundler).

For example with rollup (packager / bundler) it will import the esm packages (with import usages) (if available -- otherwise fall back to cjs etc) and rewrite them as required based on the destination code.

so for example the Application Insights code we generate es5 compatible "module" code and individual non-module code from the typescript where the dist-es5 includes the import { AppInsightsCore } from "@microsoft/xxx" but the "main" is a UMD module which supports either require/define usage or "creates" a namespaced set of code (no import insight)

    "main": "dist/es5/applicationinsights-web.js",
    "module": "dist-es5/applicationinsights-web.js",

And we support IE, even with the "module" (when using npm packages), and the packager will resolve it for you.

@paulius-valiunas
Copy link

paulius-valiunas commented Jul 14, 2023

Perhaps this is silly question. Why would web users of OTel modules be using the CommonJS in build/src/... instead of the ESM in build/esm/... for their bundled JS builds? Unless they are supporting IE and Opera Mini, or (very possibly) I don't know how to read caniuse.com/mdn-javascript_operators_import

@trentm just because a specific web browser doesn't support import statements, it doesn't mean you can use CommonJS in them. There is no browser that supports CommonJS, that's practically impossible to implement in a browser context because CommonJS requires are blocking by design. What it actually means is that in those browsers you can't reference different Javascript files from one another at all. The only two ways to include a library are to bundle everything into one file (with Webpack, Rollup etc.) or to include each file in its own <script> tag.

@klippx
Copy link

klippx commented Jul 18, 2023

I also tried upgrading my ESM project to the latest otel libs and got caught in this storm, example:

➤ YN0000: [@vcc/pricing-dgs]: import { registerInstrumentations } from '@opentelemetry/instrumentation';
➤ YN0000: [@vcc/pricing-dgs]:          ^^^^^^^^^^^^^^^^^^^^^^^^
➤ YN0000: [@vcc/pricing-dgs]: SyntaxError: Named export 'registerInstrumentations' not found. The requested module '@opentelemetry/instrumentation' is a CommonJS module, which may not support all module.exports as named exports.
➤ YN0000: [@vcc/pricing-dgs]: CommonJS modules can always be imported via the default export, for example using:
➤ YN0000: [@vcc/pricing-dgs]:
➤ YN0000: [@vcc/pricing-dgs]: import pkg from '@opentelemetry/instrumentation';
➤ YN0000: [@vcc/pricing-dgs]: const { registerInstrumentations } = pkg;

I used this trick but my project right now looking like this to cope with the 1.15.0 changes which is quite clunky (twice as many lines just to get an import straight):

import opentelemetry, { Tracer } from '@opentelemetry/api';
import { getNodeAutoInstrumentations } from '@opentelemetry/auto-instrumentations-node';
import * as OtelCoreModule from '@opentelemetry/core';
import * as OtelExporterMetricsOtlpGrpcModule from '@opentelemetry/exporter-metrics-otlp-grpc';
import * as OtelExporterPrometheusModule from '@opentelemetry/exporter-prometheus';
import * as OtelExporterTraceOtlpGrpcModule from '@opentelemetry/exporter-trace-otlp-grpc';
import * as OtelInstrumentationModule from '@opentelemetry/instrumentation';
import * as OtelInstrumentationWinstonModule from '@opentelemetry/instrumentation-winston';
import * as OtelResourcesModule from '@opentelemetry/resources';
import {
  ExplicitBucketHistogramAggregation,
  MeterProvider,
  PeriodicExportingMetricReader,
  View,
} from '@opentelemetry/sdk-metrics';
import * as OtelSDKTraceBaseModule from '@opentelemetry/sdk-trace-base';
import * as OtelSDKTraceNodeModule from '@opentelemetry/sdk-trace-node';
import * as OtelSemanticConventionsModule from '@opentelemetry/semantic-conventions';
import { FromEnv } from '../config/index.js';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { registerInstrumentations } = (OtelInstrumentationModule as any)
  .default as typeof OtelInstrumentationModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { Resource } = (OtelResourcesModule as any)
  .default as typeof OtelResourcesModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { PrometheusExporter } = (OtelExporterPrometheusModule as any)
  .default as typeof OtelExporterPrometheusModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { OTLPTraceExporter } = (OtelExporterTraceOtlpGrpcModule as any)
  .default as typeof OtelExporterTraceOtlpGrpcModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { OTLPMetricExporter } = (OtelExporterMetricsOtlpGrpcModule as any)
  .default as typeof OtelExporterMetricsOtlpGrpcModule;

const { CompositePropagator, W3CBaggagePropagator, W3CTraceContextPropagator } =
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  (OtelCoreModule as any).default as typeof OtelCoreModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { BatchSpanProcessor } = (OtelSDKTraceBaseModule as any)
  .default as typeof OtelSDKTraceBaseModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { NodeTracerProvider } = (OtelSDKTraceNodeModule as any)
  .default as typeof OtelSDKTraceNodeModule;

// https://github.com/open-telemetry/opentelemetry-js/issues/3989
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { SemanticResourceAttributes } = (OtelSemanticConventionsModule as any)
  .default as typeof OtelSemanticConventionsModule;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const { WinstonInstrumentation } = (OtelInstrumentationWinstonModule as any)
  .default as typeof OtelInstrumentationWinstonModule;

EDIT: Updated, the default issue was there for all modules and I discovered it when trying to promote to staging.

@trentm
Copy link
Contributor

trentm commented Jul 18, 2023

I have a start at a fix that replaces all export * from './{localPath}'; re-exports to explicit exports. It'll be a large diff of these:

diff --git a/packages/opentelemetry-resources/src/detectors/index.ts b/packages/opentelemetry-resources/src/detectors/index.ts
index e7d885be..0d90ebc2 100644
--- a/packages/opentelemetry-resources/src/detectors/index.ts
+++ b/packages/opentelemetry-resources/src/detectors/index.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */

-export * from './BrowserDetector';
-export * from './EnvDetector';
-export * from './BrowserDetectorSync';
-export * from './EnvDetectorSync';
+export { browserDetector } from './BrowserDetector';
+export { envDetector } from './EnvDetector';
+export { browserDetectorSync } from './BrowserDetectorSync';
+export { envDetectorSync } from './EnvDetectorSync';
diff --git a/packages/opentelemetry-resources/src/index.ts b/packages/opentelemetry-resources/src/index.ts
index 3f66901f..6be5ac0c 100644
--- a/packages/opentelemetry-resources/src/index.ts
+++ b/packages/opentelemetry-resources/src/index.ts
@@ -14,10 +14,10 @@
  * limitations under the License.
  */

-export * from './Resource';
-export * from './IResource';
-export * from './platform';
-export * from './types';
-export * from './config';
-export * from './detectors';
-export * from './detect-resources';
+export { Resource } from './Resource';
+export { IResource } from './IResource';
+export { defaultServiceName, hostDetector, osDetector, hostDetectorSync, osDetectorSync, processDetector, processDetectorSync } from './platform';
+export { ResourceAttributes, Detector, DetectorSync } from './types';
+export { ResourceDetectionConfig } from './config';
+export { browserDetector, envDetector, browserDetectorSync, envDetectorSync } from './detectors';
+export { detectResources, detectResourcesSync } from './detect-resources';

I haven't finished, however, and I'm away this week so I will make little or no progress on it, so definitely don't wait for me if a fix is wanted sooner.


A lint guard on using re-exports can be added with this:

diff --git a/eslint.base.js b/eslint.base.js
index 2ec89746..240ae727 100644
--- a/eslint.base.js
+++ b/eslint.base.js
@@ -19,6 +19,7 @@ module.exports = {
     "prefer-rest-params": "off",
     "no-console": "error",
     "no-shadow": "off",
+    "no-restricted-syntax": ["error", "ExportAllDeclaration"],
     "node/no-deprecated-api": ["warn"],

I would appreciate advice on how to add a test case to ensure that using named exports from a .mjs file works. I don't know the test suite well.

@paulius-valiunas
Copy link

paulius-valiunas commented Jul 19, 2023

I have a start at a fix that replaces all export * from './{localPath}'; re-exports to explicit exports. It'll be a large diff of these:

This looks like a maintenance nightmare. It's already enough of a burden to have to add each individual file to the barrel file. Why not fix the ESM output so it can be used in Node.js? All you have to do is add .js to each relative path import (package imports don't have to be changed) and add "type": "module" to the package.json - like described in #3989

I just don't understand why this package has ESM files in it but we can't use it even though our project is purely ESM. What you currently have is a half-baked broken ESM that only works with some bundlers that are more relaxed about enforcing the ESM standard. This reminds me of a website that's part HTML5, part XHTML and has syntax problems but no one cares because Internet Explorer "fixes" the errors by displaying whatever it thinks is right.

@klippx
Copy link

klippx commented Jul 19, 2023

I think you (maintainers) should create a small private ESM project yourself and see if it works or not, to smoke test the release candidate and see if import works or not in ESM project. Eat your own dog food, as the saying goes.

Otherwise you might as well drop the ESM support for now until that is done?

@JamieDanielson
Copy link
Member

I would appreciate advice on how to add a test case to ensure that using named exports from a .mjs file works. I don't know the test suite well.

I was able to repro this issue using the example app I added for esm-http-ts when I installed from the published package (Named export 'ConsoleSpanExporter' not found).

My thought is similar to a point in the comment above - we use this app in a smoke test. I'm not sure if we already do that or something similar elsewhere in this repo, but I can take a look at that.

@lizthegrey
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has:reproducer This bug/feature has a minimal reproducer provided priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet