ref(utils): Stop setting transaction in requestDataIntegration
#14306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
Lms24
approved these changes
Nov 14, 2024
lforst
reviewed
Nov 14, 2024
| @@ -35,6 +34,7 @@ export type AddRequestDataToEventOptions = { | |||
| include?: { | |||
| ip?: boolean; | |||
| request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>; | |||
| /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */ | |||
There was a problem hiding this comment.
Suggested change
| /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */ | |
| /** | |
| * @deprecated This option will be removed with the next major version of the SDK. If you want to disable capturing of the URL, please use the `request` option instead. | |
| */ |
maybe a bit more transparent
There was a problem hiding this comment.
but the request is quite different from this, isn't it? Opting out of the request there will not affect the transaction being set on the transaction event - though to be honest, it probably did not actually affect it even before, because that was set anyhow by other code already 😅
lforst
approved these changes
Nov 14, 2024
|
Actually, I think this means we can also deprecate |
This is not really necessary anymore - it only sets this on transaction events, and those get the `transaction` in different places already anyhow. With this, we can also actually remove some other stuff. One method is exported from utils but not otherwise used, we can also drop this in v9. Finally, this was also the only place that used `route` on the request, so we can also get rid of this in `remix`, which is weird anyhow because we set it for errors there but don't even use it for them.
mydea
force-pushed
the
fn/deprecate-transaction-method
branch
from
edcca51
to
1025797
Compare
Good point, also deprecated this! |
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not really necessary anymore - it only sets this on transaction events, and those get the
transactionin different places already anyhow.With this, we can also actually remove some other stuff. One method is exported from utils but not otherwise used, we can also drop this in v9.
Finally, this was also the only place that used
routeon the request, so we can also get rid of this inremix, which is weird anyhow because we set it for errors there but don't even use it for them.