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

feat(detectors): generate esm build files too #2636

Merged

Conversation

serkan-ozal
Copy link
Contributor

@serkan-ozal serkan-ozal commented Jan 10, 2025

Which problem is this PR solving?

As OTEL FAAS SIG, we are working on reducing OTEL Lambda Nodejs layer coldstart overhead and bundling has the biggest impact. But bundling tools like webpack, esbuild don't like CJS modules while tree-shaking, but ESM modules. So, in this PR, we are configuring build process to generate ESM build files too.

Short description of the changes

To be able to generate ESM build files too, this PR is

  • introducing tsconfig.esm.json files
  • and updating scripts accordingly in the package.json
  • and defining ESM file paths in the files section of the package.json for their discoverability

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.79%. Comparing base (fdab5d4) to head (e6d363e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2636   +/-   ##
=======================================
  Coverage   90.79%   90.79%           
=======================================
  Files         169      169           
  Lines        8059     8059           
  Branches     1645     1645           
=======================================
  Hits         7317     7317           
  Misses        742      742           

@serkan-ozal serkan-ozal force-pushed the feat/detectors/generate-esm-builds branch from e3e0a8c to e6d363e Compare January 10, 2025 13:20
@dyladan
Copy link
Member

dyladan commented Jan 10, 2025

Looks like this is done the same way as our other ESM libraries like the aws propagator etc. You should know that this method of ESM publishing is actually not standards compliant and we've had problems with it in the past. There is an issue in the main repo to revamp our ESM publishing strategy.

For now I don't see a problem applying the strategy we already use to other packages, but you should be aware it might be changing in the near future.

@serkan-ozal
Copy link
Contributor Author

serkan-ozal commented Jan 10, 2025

@dyladan Yes, I know. I just wanted to be sync with the current approach and if it will be changed in the future, this one also can be changed along with all the others together.

@dyladan
Copy link
Member

dyladan commented Jan 10, 2025

@serkan-ozal sorry i was mostly putting that there for people in the future wondering why things might be the way they are, and for other reviewers.

@trentm trentm merged commit c2ad0af into open-telemetry:main Jan 10, 2025
25 checks passed
@dyladan dyladan mentioned this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants