Skip to content

fix(otlp-exporter-base): remove usage of require in esm/esnext builds#5746

Closed
EmrysMyrddin wants to merge 2 commits intoopen-telemetry:mainfrom
EmrysMyrddin:fix/otlp-exporter-base/remove-require
Closed

fix(otlp-exporter-base): remove usage of require in esm/esnext builds#5746
EmrysMyrddin wants to merge 2 commits intoopen-telemetry:mainfrom
EmrysMyrddin:fix/otlp-exporter-base/remove-require

Conversation

@EmrysMyrddin
Copy link
Copy Markdown

Which problem is this PR solving?

ESM and ESNEXT builds are broken, because they contain the usage of require function to dynamically import packages.

Short description of the changes

This PR is replacing the require call with an import one. The only problem of this is that it is now asynchronous.

But this is not a problem since we can change the return type of the method (it is a private method), and the caller is already async.

The only difficulty is to make sure there is no parallel loading happening.

Type of change

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

How Has This Been Tested?

I've tested it on my local project which was bundling an ESM Node application with Rollup.

Checklist:

  • Followed the style guidelines of this project
  • [ ] Unit tests have been added Not relevant, this PR doesn't add anything
  • [ ] Documentation has been updated Not relevant, it is purely internal changes

@EmrysMyrddin EmrysMyrddin requested a review from a team as a code owner June 6, 2025 14:50
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jun 6, 2025

CLA Not Signed

@EmrysMyrddin EmrysMyrddin force-pushed the fix/otlp-exporter-base/remove-require branch from 8289bcc to 00a06f5 Compare June 6, 2025 15:31
@raphael-theriault-swi
Copy link
Copy Markdown
Member

This was previously attempted in #5220 but the changes require the test runner configuration to be adjusted for tests to work. I'm currently figuring out a way to make it work in #5719 which also includes the changes you're making here, which should hopefully fix your problem :)

@EmrysMyrddin
Copy link
Copy Markdown
Author

Oh ok, It's my first time trying to POC around this code base ^^'
I searched for an open issue, but didn't thought about looking at closed PRs !

I haven't looked at how tests are made, since I wasn't expecting having to update tests for this PR :-P So not quite sure to understand how this change does break the setup ^^'

I will follow your PR then ! Do you want me to close this one ?

@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Jun 6, 2025

Has anyone looked into using module.createRequire(filename) as an option to keep require() working without moving to async logic?

@EmrysMyrddin
Copy link
Copy Markdown
Author

I'm not sure how much this would be compatible with web/non node runtimes


return utils;
// Lazy require to ensure that http/https is not required before instrumentations can wrap it.
return (this._loadingUtils = import('./http-transport-utils').then(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If dynamic import fails you may want to add catch logic to handle the resultant error.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It was not handled before, so to maintain the same behavior, I don't catch any error.
The rejected promise will be handled by the caller of send function if it needs to be handled.

@EmrysMyrddin
Copy link
Copy Markdown
Author

closing in favor of #5719 which is fixing this and have working unit tests :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants