-
Notifications
You must be signed in to change notification settings - Fork 821
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
fix: Don't use require
to load package.json
files
#4593
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4593 +/- ##
==========================================
- Coverage 92.83% 89.26% -3.58%
==========================================
Files 329 147 -182
Lines 9528 3138 -6390
Branches 2053 686 -1367
==========================================
- Hits 8845 2801 -6044
+ Misses 683 337 -346 |
Please add a changelog entry |
Done. Let me know if I've put it in the wrong changelog! |
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.
This looks good, thanks for working on this. 👍
…#4593) * fix: Don't use require to load package.json files * update changelog * Move changelog entry --------- Co-authored-by: Marc Pichler <[email protected]>
Which problem is this PR solving?
It's currently not possible to bundle
@opentelemetry/instrumentation
with many bundlers.Webpack fails because it errors with
Critical dependency: The request of a dependency is an expression
. This is because webpack can see therequire
but can't reason about what is being loaded.Rollup can only bundle the code if you include a plugin that can load json files.
Since auto instrumentation will not work when the code is bundled, why would this change even matter?
Sentry is using
@opentelemetry/instrumentation
as a dependency in their Node SDK. Some users will not care about instrumentation but will want to bundle their code and usingrequire
like this to load json files makes it impossible to use some bundlers or at a minimum requires extra bundler config to work around it.Short description of the changes
Use
readFileSync
+JSON.parse
instead.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Covered by existing tests
Checklist: