-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add logger bundle variants #18781
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
Add logger bundle variants #18781
Conversation
| @@ -0,0 +1,31 @@ | |||
| import { registerSpanErrorInstrumentation } from '@sentry/core'; | |||
| import { feedbackIntegrationShim } from '@sentry-internal/integration-shims'; | |||
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.
Bug: The index.bundle.tracing.replay.feedback.logger.ts file imports the shim but fails to export feedbackIntegration, preventing users of this bundle from initializing the feedback feature.
Severity: CRITICAL
🔍 Detailed Analysis
The bundle file index.bundle.tracing.replay.feedback.logger.ts is intended to provide full feedback functionality, as indicated by its name. However, while it exports helper functions like getFeedback and sendFeedback, it incorrectly imports feedbackIntegrationShim and fails to export the main feedbackIntegration or feedbackAsyncIntegration. Consequently, users attempting to initialize Sentry with Sentry.feedbackIntegration() in this bundle will encounter a runtime error because the function will be undefined, making the feedback feature unusable.
💡 Suggested Fix
In index.bundle.tracing.replay.feedback.logger.ts, replace the import of feedbackIntegrationShim with feedbackAsyncIntegration from ./feedbackAsync. Then, add an export statement like export { feedbackAsyncIntegration as feedbackAsyncIntegration, feedbackAsyncIntegration as feedbackIntegration } to correctly expose the integration.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/browser/src/index.bundle.tracing.replay.feedback.logger.ts#L2
Potential issue: The bundle file `index.bundle.tracing.replay.feedback.logger.ts` is
intended to provide full feedback functionality, as indicated by its name. However,
while it exports helper functions like `getFeedback` and `sendFeedback`, it incorrectly
imports `feedbackIntegrationShim` and fails to export the main `feedbackIntegration` or
`feedbackAsyncIntegration`. Consequently, users attempting to initialize Sentry with
`Sentry.feedbackIntegration()` in this bundle will encounter a runtime error because the
function will be undefined, making the feedback feature unusable.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8475823
|
|
||
| export { getFeedback, sendFeedback } from '@sentry-internal/feedback'; | ||
|
|
||
| export { replayIntegration, getReplay } from '@sentry-internal/replay'; |
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.
Missing feedback integration exports in tracing.replay.feedback.logger bundle
High Severity
The bundle.tracing.replay.feedback.logger bundle is missing the feedback integration exports. It imports feedbackIntegrationShim but never uses it, and it doesn't import feedbackAsyncIntegration from ./feedbackAsync or export feedbackAsyncIntegration/feedbackIntegration. Comparing with index.bundle.tracing.replay.feedback.ts, this file needs to import feedbackAsyncIntegration and export it as both feedbackAsyncIntegration and feedbackIntegration. Without this, users of this bundle won't have access to the feedback integration despite the bundle name suggesting otherwise.
| feedbackIntegrationShim as feedbackAsyncIntegration, | ||
| feedbackIntegrationShim as feedbackIntegration, | ||
| replayIntegrationShim as replayIntegration, | ||
| }; |
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.
Missing integration or E2E tests for new feature
Low Severity · BugBot Rules
Per the rules file in the user instructions: "When reviewing a feat PR, check if the PR includes at least one integration or E2E test. If neither of the two are present, add a comment, recommending to add one." This PR adds 7 new logger bundle variants but doesn't include any integration or E2E tests to verify the bundles work correctly and export the expected APIs. Adding tests would help catch issues like the missing feedback integration exports in the tracing.replay.feedback.logger bundle.
|
Hey @behnammodi, first of all thanks for contributing! We're currently working on this internally to accommodate also for metrics + keep the permutations to a minimum. Due to this I will have to close your pr, sorry for the inconvenience! |
Adds 7 new logger bundle variants to
@sentry/browserthat combine the logger integration with various other features (tracing, replay, feedback).Bundles Added
bundle.loggerbundle.tracing.loggerbundle.replay.loggerbundle.feedback.loggerbundle.tracing.replay.loggerbundle.replay.feedback.loggerbundle.tracing.replay.feedback.loggerBefore submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #issue_link_here