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

ref(node): Small RequestData integration tweaks #5979

Merged
merged 7 commits into from
Oct 19, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Oct 17, 2022

This makes a few small revisions to the new RequestData integration:

  • 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 RequestDataOptions type to RequestDataIntegrationOptions, to help distinguish it from the many other flavors of request data functions and types floating around.

  • Make all properties in RequestDataIntegrationOptions optional, rather than using the Partial helper 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 AddRequestDataToEventOptions from 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 include option for use in addRequestDataToEvent to more specifically indicate that it's converting from integration-style options to addRequestDataToEvent-style options.

  • Refactor the aforementioned helper function to act upon and return an entire options object rather than just the include property, in order to have access to the transactionNamingScheme option.

  • Add missing transaction property in helper function's output.

  • Add tests for the helper function.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.41 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.09 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.03 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.44 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.79 KB (0%)
@sentry/browser - Webpack (minified) 64.93 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.81 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.53 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.86 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.28 KB (-0.01% 🔽)

@lobsterkatie lobsterkatie requested review from a team, lforst and Lms24 and removed request for a team October 17, 2022 22:43
};

/** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
transactionNamingScheme: TransactionNamingScheme;
transactionNamingScheme?: TransactionNamingScheme;

/**
* 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.
Copy link
Member

Choose a reason for hiding this comment

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

l: this is no longer true right? Shall we update?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which part is no longer true?

Copy link
Member

Choose a reason for hiding this comment

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

We are no longer moving the integration to core right? We are just making seperate ones for browser/node.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@lobsterkatie lobsterkatie force-pushed the kmclb-node-clean-up-RequestData-integration branch from f1f94c6 to 2ec8261 Compare October 18, 2022 13:59
@lobsterkatie lobsterkatie force-pushed the kmclb-node-clean-up-RequestData-integration branch from 2ec8261 to 2e54082 Compare October 18, 2022 23:11
@lobsterkatie lobsterkatie force-pushed the kmclb-node-clean-up-RequestData-integration branch from 2e54082 to 9e9ee94 Compare October 18, 2022 23:24
@lobsterkatie lobsterkatie merged commit c1d9c66 into master Oct 19, 2022
@lobsterkatie lobsterkatie deleted the kmclb-node-clean-up-RequestData-integration branch October 19, 2022 06:38
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.

2 participants