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

deps(opentelemetry-instrumentation): Bump shimmer types to 1.2.0 #4865

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Jul 15, 2024

Which problem is this PR solving?

Bumps the shimmer package to a version that doesn't include an invasive type that breaks certain setups.

See PR for more details: DefinitelyTyped/DefinitelyTyped#69966

Fixes getsentry/sentry-javascript#12682

This PR supersedes #4835

Short description of the changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I don't think tests are necessary here. Feel free to let me know though!

Checklist:

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

@lforst lforst requested a review from a team as a code owner July 15, 2024 11:13
@@ -38727,7 +38727,7 @@
"version": "file:experimental/packages/opentelemetry-instrumentation-grpc",
"requires": {
"@bufbuild/buf": "1.21.0-1",
"@grpc/grpc-js": "^1.10.10",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were stale?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh... so that arguable staleness came from this renovatebot change: #4822
which, IIUC, is normally fixed up by a regular "lock file maintenance" PR.
This one: #4834
That one does indeed make this same kind of cleanup in the lock file. However, there is another issue (in an unrelated package) that is blocking that #4834.

I've been leaning on @pichlermarc's confidence in the "lock file maintenance" thing being sufficient. Marc's on vacation for a while, so I'm not sure the best course of action.

I assume you got these package-lock changes from just running npm update @types/shimmer or npm install @types/shimmer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. This is just from a normal npm i. If this is fixed in the main branch I can just rebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is fixed in main yet. See the "discussion" at #4834.
I think taking the package-lock changes you have in this PR would likely be fine, but all-things-being-equal I'd tend to wait for Marc's opinion. I'll discuss with some others tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds perfectly reasonable to me! :) Thanks for taking the time!

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed in the OTel JS SIG today (https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit#heading=h.7awcc4kiwbis). Discussion veered widely into TypeScript versions used in OTel JS, but for this issue the SIG notes have a succinct "doit". :)

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.

Sentry sdk adds "__wrapped"-properties to types, which prevents nestjs project from building
2 participants