opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

fix(api): esnext export condition is more specific than module

Open cjpearson opened this issue 9 months ago • 7 comments
trafficstars

Which problem is this PR solving?

I'm building an application with rollup. I want to use the esnext export from @opentelemetry/api, so I add 'esnext' as an export condition. However, since the 'module' condition is always used and in the package.json it occurs before the esnext condition, the module condition will always match.

Since the esnext condition is more specific, I think it makes sense to have it occur before the module condition.

https://nodejs.org/api/packages.html#packages_conditional_exports

Short description of the changes

Move esnext export condition to the top in package.json, so that an esnext + module condition will resolve to the esnext directory.

Before exportConditions: ['module', 'esnext'] => esm exportConditions: ['module'] => esm

After exportConditions: ['module', 'esnext'] => esnext exportConditions: ['module'] => esm

Type of change

Please delete options that are not relevant.

  • [x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [x] Local testing

Checklist:

  • [x] Followed the style guidelines of this project
  • [ ] Unit tests have been added
  • [ ] Documentation has been updated

cjpearson avatar Feb 12 '25 16:02 cjpearson

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 95.01%. Comparing base (41ba7f5) to head (d365bc8). :warning: Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5458   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files         303      303           
  Lines        7946     7946           
  Branches     1607     1607           
=======================================
  Hits         7550     7550           
  Misses        396      396           
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 12 '25 16:02 codecov[bot]

@cjpearson Interestingly we were (separately) discussing dropping the build/esnext export... though that was about packages other than this @opentelemetry/api package, and in context of bumping the TypeScript transpilation target to ES2022 (https://github.com/open-telemetry/opentelemetry-js/issues/5393).

Can you show a repro, or at least your full rollup config? I'm not very familiar with the config settings involved in the various bundlers here.

Also, I'm curious what about the build/esnext output that you want/need vs. the current build/esm output. Thanks.

trentm avatar Feb 12 '25 17:02 trentm

I wanted to use the esnext build to get the newer compilation target, since rollup also complains about the global this that is present in the esm build. So bumping the target for esm would be even better, but I thought this might work as a non-breaking solution.

I'm not actually using rollup directly, but rather through nuxt. I'll try and create a simple reproduction though.

cjpearson avatar Feb 12 '25 18:02 cjpearson

I've created an example here: https://github.com/cjpearson/nuxt-otel-repro

Nuxt uses several plugins, so it's probably possible to narrow it down further if needed. But this app adds a single endpoint which imports trace from @opentelemetry/api and creates a span.

When building, rollup logs a bunch of warnings:

ℹ Building Nuxt Nitro server (preset: node-server, compatibility date: 2024-11-01)                                                                                                    
node_modules/@opentelemetry/api/build/esm/metrics/NoopMeter.js (16:17): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/metrics/NoopMeter.js (16:25): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/diag.js (16:14): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/diag.js (16:22): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/diag.js (32:21): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/diag.js (32:29): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/context.js (16:14): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/context.js (16:22): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/context.js (32:21): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/api/context.js (32:29): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/baggage/internal/baggage-impl.js (16:14): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/baggage/internal/baggage-impl.js (16:22): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/baggage/internal/baggage-impl.js (32:16): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/baggage/internal/baggage-impl.js (32:24): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/diag/ComponentLogger.js (16:14): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/diag/ComponentLogger.js (16:22): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/diag/ComponentLogger.js (32:21): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/diag/ComponentLogger.js (32:29): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js (16:14): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js (16:22): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js (32:21): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten
node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js (32:29): The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten

cjpearson avatar Feb 12 '25 18:02 cjpearson

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Apr 14 '25 06:04 github-actions[bot]

not stale

cjpearson avatar Apr 14 '25 07:04 cjpearson

Hi @trentm, would it be possible to take another look at this?

cjpearson avatar Jun 03 '25 17:06 cjpearson

Assuming this is the right thing to do, we theoretically should do this for the other packages in the repo that have similar "exports":

experimental/packages/otlp-exporter-base/package.json
9:  "exports": {

semantic-conventions/package.json
9:  "exports": {

trentm avatar Jul 09 '25 16:07 trentm

I think that makes sense. I've updated the PR to also include the changes for those packages

cjpearson avatar Jul 11 '25 13:07 cjpearson

I raised this with the browser group to try to get a bundler expert opinion on this. It looks ok to me but I don't have a lot of experience with the intricacies of different bundlers and their behaviors. I don't want to accidentally do something to fix one problem that causes another.

dyladan avatar Jul 11 '25 15:07 dyladan

@dyladan @cjpearson Tested with rollup 4.45.1 (latest). Thanks for doing this.

overbalance avatar Jul 24 '25 20:07 overbalance

@cjpearson @dyladan is anything else needed to get this merged?

overbalance avatar Jul 29 '25 15:07 overbalance

Thank you for your contribution @cjpearson! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

opentelemetrybot avatar Aug 06 '25 15:08 opentelemetrybot