fix(api): esnext export condition is more specific than module#5458
fix(api): esnext export condition is more specific than module#5458pichlermarc merged 1 commit intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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 🚀 New features to boost your workflow:
|
|
@cjpearson Interestingly we were (separately) discussing dropping the 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 |
|
I wanted to use the esnext build to get the newer compilation target, since rollup also complains about the global I'm not actually using rollup directly, but rather through nuxt. I'll try and create a simple reproduction though. |
|
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 When building, rollup logs a bunch of warnings: |
|
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. |
|
not stale |
|
Hi @trentm, would it be possible to take another look at this? |
|
Assuming this is the right thing to do, we theoretically should do this for the other packages in the repo that have similar "exports": |
7428952 to
d365bc8
Compare
|
I think that makes sense. I've updated the PR to also include the changes for those packages |
|
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 @cjpearson Tested with rollup 4.45.1 (latest). Thanks for doing this. |
dyladan
left a comment
There was a problem hiding this comment.
We can't let this sit forever and I think it's unlikely to have unintended consequences. LGTM
|
@cjpearson @dyladan is anything else needed to get this merged? |
|
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. |
Which problem is this PR solving?
I'm building an application with rollup. I want to use the
esnextexport 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']=>esmexportConditions: ['module']=>esmAfter
exportConditions: ['module', 'esnext']=>esnextexportConditions: ['module']=>esmType of change
Please delete options that are not relevant.
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
Checklist: