Skip to content
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(build): Enable esModuleInterop for improved compatibility #4519

Closed
wants to merge 3 commits into from

Conversation

Ankcorn
Copy link

@Ankcorn Ankcorn commented Mar 1, 2024

Which problem is this PR solving?

I ran into issues instrumenting code with the import-in-the-middle package whilst using the bundler esbuild (to bundle my telemetry code - not modules I wanted to trace). I noticed that the import looked like

import * as ImportInTheMiddle from 'import-in-the-middle'

As flagged in issue #3954 this relies on some incorrect assumptions typescript makes about module bundling.

A detailed description of these wrong assumptions can be found here: https://www.typescriptlang.org/tsconfig#esModuleInterop.

The tldr is import * as moment from "moment" is not the same as const moment = require("moment")

Turning on esModuleInterop fixes these problems but required a few updates across the codebase that I believe will not change the behavior of the packages
Fixes #3954

Short description of the changes

Turned on esModuleInterop in the base tsconfig.

Fixed the imports so that where the default import is desired we import Mod from 'mod' and when the module doesn't have a default import we stick with import * as Mod from 'mod'

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

I have ran the tests and tscompiler to check that things still work. I still would like to import this into an existing project using opentelemetry and check there are no issues. I'm not familiar with doing this on a project so large - guidance welcome here 😄

Checklist:

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

@Ankcorn Ankcorn requested a review from a team March 1, 2024 13:02
@Ankcorn
Copy link
Author

Ankcorn commented Mar 14, 2024

@pichlermarc @dyladan Would you be able to take a look at this 😁 Sorry to nag! Been speaking to lots of people who have been burnt by this issue of the last few weeks

@dyladan
Copy link
Member

dyladan commented Mar 14, 2024

This has come up in the past a couple times. If I remember correctly, if we enable esModuleInterop it also requires our downstream consumers to enable it, which we do not consider acceptable. You can actually see other projects have disabled esModuleInterop for exactly this reason https://github.com/react-native-modal/react-native-modal/pull/371/files

Looking at this linked issue, i think this is actually a better solution:

I solved the problem for my local environment by editing the import deep in down in node_modules. I found import * as ImportInTheMiddle from 'import-in-the-middle'; and changed it to import ImportInTheMiddle from 'import-in-the-middle';.

@Ankcorn
Copy link
Author

Ankcorn commented Mar 14, 2024

Thanks that totally makes sense, I was not aware it would break downstream packages because opentelemetry-js ships the bundled code.

Your solution avoids this, thanks for figuring this out ❤️

Copy link

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 github-actions bot added the stale label May 20, 2024
Copy link

github-actions bot commented Jun 3, 2024

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong import for "import-in-the-middle"
2 participants