-
Notifications
You must be signed in to change notification settings - Fork 932
refactor(otlp-transformer): generate and export esm protos #5925
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
refactor(otlp-transformer): generate and export esm protos #5925
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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:
|
f6518ee
to
bc56727
Compare
bc56727
to
4495b05
Compare
There was a problem hiding this 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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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 😅)
There was a problem hiding this comment.
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 😎
Closing in favor of choosing a real ESM replacement. |
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.Short description of the changes
This PR generates an additional ESM file
./experimental/packages/otlp-transformer/src/generated/root.mjs
and copies it into thebuild/esnext
andbuild/esm
folders asroot.js
.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: