feat(feedback): Maintain v7 compat in the @sentry-internal/feedback package #11461
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 📦
|
…w, and hooks for screenshots, which were missing before
mydea
reviewed
Apr 9, 2024
| @@ -141,7 +142,8 @@ export const feedbackIntegration = (({ | |||
| if (!client) { | |||
| throw new Error('Sentry Client is not initialized correctly'); | |||
| } | |||
| const modalIntegration = client.getIntegrationByName<FeedbackModalIntegration>('FeedbackModal'); | |||
There was a problem hiding this comment.
should we combine this? So e.g.:
const modalIntegration = client.getIntegrationByName<FeedbackModalIntegration>('FeedbackModal');
// If the user has not added it themselves yet, we add the integration for them
if (!modalIntegration) {
const modalIntegration: FeedbackModalIntegration = feedbackModalIntegration();
client.addIntegration(modalIntegration);
}There was a problem hiding this comment.
Is it a problem to re-add it if it's already been added? This integration especially has no state, we're using it to pass around some function references
There was a problem hiding this comment.
Actually, I'm going to leave it because with out desired interface the user shouldn't be manually adding things (they never did, never need to).
This spot is where the call to Sentry.lazyLoadIntegration() will happen, so we'll tweak this whole code chunk and include that lazyLoad in one swoop in a followup PR
mydea
approved these changes
Apr 9, 2024
c298lee
approved these changes
Apr 9, 2024
c298lee
reviewed
Apr 9, 2024
| @@ -141,7 +142,8 @@ export const feedbackIntegration = (({ | |||
| if (!client) { | |||
| throw new Error('Sentry Client is not initialized correctly'); | |||
| } | |||
| const modalIntegration = client.getIntegrationByName<FeedbackModalIntegration>('FeedbackModal'); | |||
| const modalIntegration: FeedbackModalIntegration = feedbackModalIntegration(); | |||
There was a problem hiding this comment.
why can't we use getIntegrationByName for the feedback modal integration?
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
…ackage (getsentry#11461) Proceeding along the [plan to get feedback screenshots async loading](getsentry#11435 (comment)), this PR imports the feedback modal back into the main package, so we can maintain compatibility with the v7 version of the sdk, which did the same.
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.
Proceeding along the plan to get feedback screenshots async loading, this PR imports the feedback modal back into the main package, so we can maintain compatibility with the v7 version of the sdk, which did the same.