Skip to content

Conversation

david-luna
Copy link
Contributor

@david-luna david-luna commented Jul 23, 2025

Which problem is this PR solving?

OpenTelemetry JS modules do not have valid ESM files in the compilation result. See open-telemetry/opentelemetry-js#3989

This PR is an attempt to fix it with adding a new tool named tsup which compiles the code with esbuild under the hood.
The scope is reduced to @opentelemetry/instrumentation-undici because this implies changes in the toolchain and needs to be discussed in the Javascript SIG

Short description of the changes

  • Added tsup dependency
  • changed compile script to use tsup
  • changed the package.json to get the proper mapping foe cjs and esm modules
    • add "exports" section
    • modify files section

NOTES:

  • this add a new tool in the repo (esbuild) which is faster but it does not check the types (actually the reason why it's faster)
  • test scripts still use tsc but IMO it is odd to have a type error it in compilation time but in test
  • tsup does emit, in this case, a single file with all the instrumentation code instead of emit a single file per .ts file. Not sure if this is a good or bad thing

Notice

This PR is meant to be reviewed along with #2955 which does the same but using the current toolchain of the repo. The intention is to have a conversation on the path we want to take about tooling. Should we stick to the current tools (for compilation) or change them?

Copy link

codecov bot commented Jul 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@215c2b5). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2954   +/-   ##
=======================================
  Coverage        ?   89.01%           
=======================================
  Files           ?      188           
  Lines           ?     9221           
  Branches        ?     1900           
=======================================
  Hits            ?     8208           
  Misses          ?     1013           
  Partials        ?        0           
🚀 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.

@david-luna david-luna changed the title chore: use tsup for cjs,esm output chore(instr-undici): use tsup for cjs,esm output Jul 23, 2025
@JamieDanielson
Copy link
Member

@t2t2
Copy link
Contributor

t2t2 commented Jul 23, 2025

tsup does emit, in this case, a single file with all the instrumentation code instead of emit a single file per .ts file. Not sure if this is a good or bad thing

esbuild has a mentioned-in-documentation-but-never-explained config option bundle for this, so adding bundle: false, (--no-bundle ?) will emit them separately again (if desired - considering the potential upsides for bundling were mentioned in the sig)

...however there's a bug in tsup where it either drops or puts a wrong type of file extension in requires/imports (either .mjs trying to load extensionless .js (commonjs) or (when package.json has type: module) .cjs files trying to load .js (esm)) - this snippet has worked for me in the second case but it requires a config file to add a plugin

@david-luna
Copy link
Contributor Author

@t2t2 thanks for the info about esbuild bundle option :)

In last JS SIG (2025-07-23) there was a comment about the potential performance benefit of having the code in a single file. So I'll keep the current options for now.

This PR is now ready to compare with #2955. I'll add the no-bundle option if it's required for the discussion on which approach we should take.

@david-luna david-luna marked this pull request as ready for review July 24, 2025 17:01
@david-luna david-luna requested a review from a team as a code owner July 24, 2025 17:01
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM. Looks a lot easier than I expected thankfully

Comment on lines +25 to +26
"{projectRoot}/build",
"{projectRoot}/dist"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we need both build and dist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because I couldn't find an option to change the destination dir in tsup. So for now nx needs to cache build and dist. If we move forward with this all packages will have the compilation output to dist so the bukd folder will disappear from this config.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Sorry meant to mark this as approved before not just "comment"

@david-luna
Copy link
Contributor Author

Sorry meant to mark this as approved before not just "comment"

Does that mean you prefer this solution over #2955?

@pichlermarc
Copy link
Member

@david-luna do you think it would be feasible to use esbuild and rollup directly for this change. That would cut down on one extra dependency, but would likely require a few more config lines.

@trentm
Copy link
Contributor

trentm commented Aug 13, 2025

There was some offline discussion on your two PRs, but not hard decision on preference for either of your PRs yet.

@trentm
Copy link
Contributor

trentm commented Aug 13, 2025

do you think it would be feasible to use esbuild and rollup directly for this change.

A possible example of doing so manually might be (I haven't tested): https://github.com/open-feature/js-sdk/blob/dc1970e717a8cbe7261d52d993c8811059718bed/packages/server/package.json#L21-L23

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.

6 participants