ref(node): Small RequestData integration tweaks#5979
Merged
lobsterkatie merged 7 commits intomasterfrom Oct 19, 2022
Merged
Conversation
Contributor
size-limit report 📦
|
AbhiPrasad
approved these changes
Oct 18, 2022
|
|
||
| /** | ||
| * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but | ||
| * left injectable so this integration can be moved to `@sentry/core` and used in browser-based SDKs in the future. |
Contributor
There was a problem hiding this comment.
l: this is no longer true right? Shall we update?
Member
Author
There was a problem hiding this comment.
Which part is no longer true?
Contributor
There was a problem hiding this comment.
We are no longer moving the integration to core right? We are just making seperate ones for browser/node.
Member
Author
There was a problem hiding this comment.
Wasn't sure if we'd actually decided that or just said, "Yeah, we might end up doing separate ones, so fine to move it back into the node SDK for now." Anyway, I can move it to being a property, so it's not cluttering options but could be overridden by subclassing if we ever get that far.
f1f94c6 to
2ec8261
Compare
20 tasks
2ec8261 to
2e54082
Compare
2e54082 to
9e9ee94
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 makes a few small revisions to the new
RequestDataintegration:Switch to using booleans rather than an array of keys when specifying what user data to include. This makes it match all of the other options, and is something I should have just done from the get-go. Given that the integration is new and thus far entirely undocumented, IMHO it feels safe to make what is technically a breaking change here.
Rename the integration's internal
RequestDataOptionstype toRequestDataIntegrationOptions, to help distinguish it from the many other flavors of request data functions and types floating around.Make all properties in
RequestDataIntegrationOptionsoptional, rather than using thePartialhelper type.Switch the callback which actually does the data adding from being an option to being a protected property, in order to make it less public but still leave open the option of subclassing and setting it to a different value if we ever get around to using this in browser-based SDKs. Because I also made the property's type slightly more generic and used an index signature to do it, I also had to switch
AddRequestDataToEventOptionsfrom being an interface to being a type. See Index signature is missing in type (only on interfaces, not on type alias) microsoft/TypeScript#15300.Rename the helper function which formats the
includeoption for use inaddRequestDataToEventto more specifically indicate that it's converting from integration-style options toaddRequestDataToEvent-style options.Refactor the aforementioned helper function to act upon and return an entire options object rather than just the
includeproperty, in order to have access to thetransactionNamingSchemeoption.Add missing
transactionproperty in helper function's output.Add tests for the helper function.