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: Update import-in-the-middle #4745

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jun 1, 2024

import-in-the-middle v1.8.0 has a lot of bug fixes.

Closes #4547

In one of those PRs I added a named Hook export which should work around the default export issue which closes #4691 and closes #4717.

Unfortunately when I added the export I forgot to add it to the types so the types need fudging until this PR makes it into a release.

@timfish timfish requested a review from a team June 1, 2024 12:10
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Can we add a test that validates the named hook export so this doesn't regress?

getsentry/sentry-javascript#12009 (comment) was an example of a esbuild config that had troubles with this before.

@timfish
Copy link
Contributor Author

timfish commented Jun 1, 2024

There's an open issue to add bundler tests to ensure bundler issues aren't introduced:
#4744

Open telemetry depends on a fixed version of import-in-the-moddle, so as long as the tests in this repo pass and actually test instrumentation, combined with proper bundler tests, that should probably be enough?

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

so as long as the tests in this repo pass and actually test instrumentation

Yeah that seems fair to me, I think waiting for the bundler tests is a good idea!

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for this PR and taking the time to address all of these issues upstream @timfish

The tests proposed in #4744 would indeed add a lot of coverage for these cases where bundlers are used. Contributions to help with that are very welcome.

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Marc Pichler <[email protected]>
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Happy to wait for some bundler tests, but I don't think we have to wait to merge this.

@pichlermarc
Copy link
Member

Happy to wait for some bundler tests, but I don't think we have to wait to merge this.

Agreed.

@pichlermarc pichlermarc added this pull request to the merge queue Jun 5, 2024
Merged via the queue into open-telemetry:main with commit 106fa9b Jun 5, 2024
19 checks passed
@jd-carroll
Copy link

@timfish In the fix you left the import as import * as ImportInTheMiddle instead of import { Hook }. My assumption is that there is a reason and my understanding has some holes.

If there is a reason, beyond keeping changes to a minimum, could you point me to where I could understand more?

Thanks!

@timfish
Copy link
Contributor Author

timfish commented Jun 5, 2024

When I added the named export to iitm, I forgot to update the separate types and this felt like the easiest way to cast the types for now.

Once the open iitm types PR has been merged and released I would prefer the import to be:

import { Hook } from 'import-in-the-middle';

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* fix: Update `import-in-the-middle`

* changelog and lint

* lint

* changes from code review

Co-authored-by: Marc Pichler <[email protected]>

---------

Co-authored-by: Marc Pichler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants