Skip to content

Conversation

overbalance
Copy link
Contributor

@overbalance overbalance commented Sep 12, 2025

Which problem is this PR solving?

The ESM version of otlp-transformer includes ./experimental/packages/otlp-transformer/src/generated/root.js containing the compiled output of the protos. This is generated by ./scripts/generate-protos.js in CommonJS format.

// root.js
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var $protobuf = require("protobufjs/minimal");
....

Short description of the changes

This PR generates an additional ESM file ./experimental/packages/otlp-transformer/src/generated/root.mjs and copies it into the build/esnext and build/esm folders as root.js.

// root.mjs
import * as $protobuf from "protobufjs/minimal";
....

I know protobufjs/minimal is still CommonJS, but figured we should modernize what we can on our end.

Type of change

I marked as refactor since it's not providing new functionality. Please advise if I should change it.

How Has This Been Tested?

npm run compile then check the build folders for the appropriate file.

Checklist:

  • Followed the style guidelines of this project

Copy link

codecov bot commented Sep 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.17%. Comparing base (3dc8979) to head (4495b05).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5925   +/-   ##
=======================================
  Coverage   95.17%   95.17%           
=======================================
  Files         316      316           
  Lines        8521     8521           
  Branches     1763     1763           
=======================================
  Hits         8110     8110           
  Misses        411      411           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@overbalance overbalance marked this pull request as ready for review September 12, 2025 07:30
@overbalance overbalance requested a review from a team as a code owner September 12, 2025 07:30
@overbalance overbalance force-pushed the overbalance/generate-esm-protos branch 5 times, most recently from f6518ee to bc56727 Compare September 17, 2025 15:54
@overbalance overbalance force-pushed the overbalance/generate-esm-protos branch from bc56727 to 4495b05 Compare September 23, 2025 19:02
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Approach overall idea is sound and works when the files are where they are supposed to be 🙂 I tested it using rollup and Node.js since Node.js always falls back to CJS if there's no "type": "module" entry.

However, the way the script is set up right now it does not actually copy it to the correct location when running npm run compile from the top-level.

"prepublishOnly": "npm run compile",
"precompile": "lerna run version --scope @opentelemetry/otlp-transformer --include-dependencies",
"compile": "npm run protos && tsc --build tsconfig.json tsconfig.esm.json tsconfig.esnext.json",
"compile": "npm run protos && tsc --build tsconfig.json tsconfig.esm.json tsconfig.esnext.json && npm run copy-esm-protos-to-build",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top-level compile script actually does not call this one - the result of that is that the published package won't have the generated ESM copied to the right locations, and the CJS one will be included.

(To reproduce: run npm run compile from the top-level of the repo and inspect the experimental/otlp-transformer/build/esm/generated/root.js)

"precompile": "lerna run version --scope @opentelemetry/otlp-transformer --include-dependencies",
"compile": "npm run protos && tsc --build tsconfig.json tsconfig.esm.json tsconfig.esnext.json",
"compile": "npm run protos && tsc --build tsconfig.json tsconfig.esm.json tsconfig.esnext.json && npm run copy-esm-protos-to-build",
"copy-esm-protos-to-build": "cp src/generated/root.mjs build/esm/generated/root.js && cp src/generated/root.mjs build/esnext/generated/root.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using cp will not work for devs on windows, I think we need to consider an alternative here
(if something is not working for windows devs it usually comes up really quickly after merging a PR 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I'll remember that in the future 😎

@overbalance
Copy link
Contributor Author

Closing in favor of choosing a real ESM replacement.

@overbalance overbalance closed this Oct 9, 2025
@overbalance overbalance deleted the overbalance/generate-esm-protos branch October 10, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants