-
Notifications
You must be signed in to change notification settings - Fork 132
chore(deps): support sentry v10 #2715
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
Conversation
|
Thanks for the PR! Could you also add a changeset? |
packages/plugins/sentry/package.json
Outdated
| "peerDependencies": { | ||
| "@envelop/core": "workspace:^", | ||
| "@sentry/node": "^8.0.0", | ||
| "@sentry/core": "^10.17.0", |
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.
I think the code is still compatible with v8 and v9 right? So we can keep v8 and v9 in peerDependencies?
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.
I don't know we removed global import of sentry/tracing which is now embedded in sentry/node. the type also are not compatible even if runtimewise it's not a problem.
Not sure we can consider it as compatible.
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.
If so, let's make it a major release then because it removes the compatibility of older Sentry versions. chore(deps): sentry: Update sentry to ^10.17 by thomasleduc · Pull Request #2715 · graphql-hive/envelop
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.
Yes you're right.
I have another thing to check. A parameter not tested yet in the plugin and that I don't use seems broken.
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.
TraceparentData is the old way to use Sentry.
Should I kept the original option name (tranceparentData) for minimal change, even if I change the shape for
something like { sentryTrace?: string; baggage?: string } instead of the old TraceparentData ?
a814729 to
8dce695
Compare
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
Update sentry plugin to be used with sentry v10 (before sentry v8).
Since Sentry SDK v10, you don’t need to import '@sentry/tracing' anymore.
Remove the imports in documentation and test, already embedded in sentry/node or sentry/nextjs.
sentry/types
Remove sentry/types (deprecated) and install sentry/core where the types can be found now.
Type of change
expected)
There is a breaking change since sentry is in peerDependencies and need to be updated to 10.
How Has This Been Tested?
The tests didn't changed and cover the same amount of code.
Test Environment:
@envelop/sentry:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
There is a discussion to address on whether we do a sentry/node | sentry/nextjs version of envelop/sentry plugin or not.
Now that sentry is embedding a lot in those imports, users may want to have only sentry/nextjs or sentry/node that do basically the same thing.
I choose to keep sentry/node for this plugin as it was before.