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.

Sign up for GitHub

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

feat(feedback): Maintain v7 compat in the @sentry-internal/feedback package #11461

Merged
merged 5 commits into from
Apr 9, 2024
Merged

feat(feedback): Maintain v7 compat in the @sentry-internal/feedback package #11461

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1035e50
feat(feedback): Maintain v7 compat in the @sentry-internal/feedback p…
ryan953 Apr 6, 2024
1b454ae
Merge branch 'develop' into ryan953/feedback-v7-compat
ryan953 Apr 8, 2024
24e41e2
increase size-limit for feedback -> we are bundling preact in here no…
ryan953 Apr 8, 2024
fe18c0e
also everything bundle
ryan953 Apr 8, 2024
203f6b8
another size-limit to break through
ryan953 Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .size-limit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,28 +52,28 @@ module.exports = [
path: 'packages/browser/build/npm/esm/index.js',
import: createImport('init', 'browserTracingIntegration', 'replayIntegration', 'feedbackIntegration'),
gzip: true,
limit: '80 KB',
limit: '83 KB',
},
{
name: '@sentry/browser (incl. Feedback)',
path: 'packages/browser/build/npm/esm/index.js',
import: createImport('init', 'feedbackIntegration'),
gzip: true,
limit: '35 KB',
limit: '37 KB',
},
{
name: '@sentry/browser (incl. Feedback, Feedback Modal)',
path: 'packages/browser/build/npm/esm/index.js',
import: createImport('init', 'feedbackIntegration', 'feedbackModalIntegration'),
gzip: true,
limit: '35 KB',
limit: '37 KB',
},
{
name: '@sentry/browser (incl. Feedback, Feedback Modal, Feedback Screenshot)',
path: 'packages/browser/build/npm/esm/index.js',
import: createImport('init', 'feedbackIntegration', 'feedbackModalIntegration', 'feedbackScreenshotIntegration'),
gzip: true,
limit: '35 KB',
limit: '37 KB',
},
{
name: '@sentry/browser (incl. sendFeedback)',
Expand Down
6 changes: 4 additions & 2 deletions packages/feedback/src/core/integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
SUBMIT_BUTTON_LABEL,
SUCCESS_MESSAGE_TEXT,
} from '../constants';
import { feedbackModalIntegration } from '../modal/integration';
import { DEBUG_BUILD } from '../util/debug-build';
import { isScreenshotSupported } from '../util/isScreenshotSupported';
import { mergeOptions } from '../util/mergeOptions';
Expand All @@ -43,7 +44,7 @@ export const feedbackIntegration = (({
autoInject = true,
showEmail = true,
showName = true,
showScreenshot = true,
showScreenshot = false,
useSentryUser = {
email: 'email',
name: 'username',
Expand Down Expand Up @@ -141,7 +142,8 @@ export const feedbackIntegration = (({
if (!client) {
throw new Error('Sentry Client is not initialized correctly');
}
const modalIntegration = client.getIntegrationByName<FeedbackModalIntegration>('FeedbackModal');
Copy link
Member

Choose a reason for hiding this comment

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

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);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ryan953 ryan953 Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

const modalIntegration: FeedbackModalIntegration = feedbackModalIntegration();
Copy link
Member

Choose a reason for hiding this comment

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

why can't we use getIntegrationByName for the feedback modal integration?

client.addIntegration(modalIntegration);
const screenshotIntegration = client.getIntegrationByName<FeedbackScreenshotIntegration>('FeedbackScreenshot');
const screenshotIsSupported = isScreenshotSupported();

Expand Down
Loading