Skip to content

Commit c49c9f3

Browse files
ryan953ryan953
authored
fix(feedback): Inject preact from feedbackModal into feedbackScreenshot integration (#12535)
I believe the error we're seeing in #12534 is from having preact bundled twice: once into the Modal integration, and then again into the Screenshot one. This PR updates the Screenshot code so that it requires preact to be injected into it, which should reduce the bundle size of that integration, but also solve the bug because there will only be one instance of preact and it's state within the sdk. Fixes #12534
1 parent 70e942d commit c49c9f3

File tree

9 files changed

+129
-112
lines changed

9 files changed

+129
-112
lines changed

‎docs/migration/feedback.md‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,15 @@ Below you can find a list of relevant feedback changes and issues that have been
1414
We have streamlined the interface for interacting with the Feedback widget. Below is a list of public functions that
1515
existed in 7.x and a description of how they have changed in v8.
1616

17-
| Method Name | Replacement | Notes |
18-
| ------------------------------------------------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
19-
| `Sentry.getClient<BrowserClient>()?.getIntegration(Feedback)` | `const feedback = Sentry.getFeedback()` | Get a type-safe reference to the configured feedbackIntegration instance. |
20-
| `feedback.getWidget()` | `const widget = feedback.createWidget(); widget.appendToDom()` | The SDK no longer maintains a stack of form instances. If you call `createWidget()` a new widget will be inserted into the DOM and an `ActorComponent` returned allows you control over the lifecycle of the widget. |
21-
| `feedback.openDialog()` | `widget.open()` | Make the form inside the widget visible. |
22-
| `feedback.closeDialog()` | `widget.close()` | Make the form inside the widget hidden in the page. Success/Error messages will still be rendered and will hide themselves if the form was recently submitted. |
23-
| `feedback.removeWidget()` | `widget.removeFromDom()` | Remove the form and widget instance from the page. After calling this `widget.el.parentNode` will be set to null. |
24-
| `feedback.attachTo()` | `const unsubscribe = feedback.attachTo(myButtonElem)` | The `attachTo()` method will create an onClick event listener to your html element that calls `appendToDom()` and `open()`. It returns a callback to remove the event listener. |
25-
| - | `const form = await feedback.createForm()` | A new method `createForm()`, used internally by `createWidget()` and `attachTo()`, returns a `Promise<FeedbackDialog>` so you can control showing and hiding of the feedback form directly. |
17+
| Method Name | Replacement | Notes |
18+
| ------------------------------------------------------------- | -------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
19+
| `Sentry.getClient<BrowserClient>()?.getIntegration(Feedback)` | `const feedback = Sentry.getFeedback()` | Get a type-safe reference to the configured feedbackIntegration instance. |
20+
| `feedback.getWidget()` | `const widget = feedback.createWidget(); widget.appendToDom()` | The SDK no longer maintains a stack of form instances. If you call `createWidget()` a new widget will be inserted into the DOM and an `ActorComponent` returned allows you control over the lifecycle of the widget. |
21+
| `feedback.openDialog()` | `widget.open()` | Make the form inside the widget visible. |
22+
| `feedback.closeDialog()` | `widget.close()` | Make the form inside the widget hidden in the page. Success/Error messages will still be rendered and will hide themselves if the form was recently submitted. |
23+
| `feedback.removeWidget()` | `widget.removeFromDom()` | Remove the form and widget instance from the page. After calling this `widget.el.parentNode` will be set to null. |
24+
| `feedback.attachTo()` | `const unsubscribe = feedback.attachTo(myButtonElem)` | The `attachTo()` method will create an onClick event listener to your html element that calls `appendToDom()` and `open()`. It returns a callback to remove the event listener. |
25+
| - | `const form = await feedback.createForm()` | A new method `createForm()`, used internally by `createWidget()` and `attachTo()`, returns a `Promise<ReturnType<FeedbackModalIntegration['createDialog']>>` so you can control showing and hiding of the feedback form directly. |
2626

2727
### API Examples
2828

‎packages/feedback/src/core/integration.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { getClient } from '@sentry/core';
22
import type {
3-
FeedbackDialog,
43
FeedbackInternalOptions,
54
FeedbackModalIntegration,
65
FeedbackScreenshotIntegration,
@@ -56,7 +55,9 @@ export const buildFeedbackIntegration = ({
5655
}: BuilderOptions): IntegrationFn<
5756
Integration & {
5857
attachTo(el: Element | string, optionOverrides?: OverrideFeedbackConfiguration): Unsubscribe;
59-
createForm(optionOverrides?: OverrideFeedbackConfiguration): Promise<FeedbackDialog>;
58+
createForm(
59+
optionOverrides?: OverrideFeedbackConfiguration,
60+
): Promise<ReturnType<FeedbackModalIntegration['createDialog']>>;
6061
createWidget(optionOverrides?: OverrideFeedbackConfiguration): ActorComponent;
6162
remove(): void;
6263
}
@@ -179,7 +180,9 @@ export const buildFeedbackIntegration = ({
179180
return integration as I;
180181
};
181182

182-
const _loadAndRenderDialog = async (options: FeedbackInternalOptions): Promise<FeedbackDialog> => {
183+
const _loadAndRenderDialog = async (
184+
options: FeedbackInternalOptions,
185+
): Promise<ReturnType<FeedbackModalIntegration['createDialog']>> => {
183186
const screenshotRequired = options.enableScreenshot && isScreenshotSupported();
184187
const [modalIntegration, screenshotIntegration] = await Promise.all([
185188
_findIntegration<FeedbackModalIntegration>('FeedbackModal', getModalIntegration, 'feedbackModalIntegration'),
@@ -223,7 +226,7 @@ export const buildFeedbackIntegration = ({
223226
throw new Error('Unable to attach to target element');
224227
}
225228

226-
let dialog: FeedbackDialog | null = null;
229+
let dialog: ReturnType<FeedbackModalIntegration['createDialog']> | null = null;
227230
const handleClick = async (): Promise<void> => {
228231
if (!dialog) {
229232
dialog = await _loadAndRenderDialog({
@@ -306,7 +309,9 @@ export const buildFeedbackIntegration = ({
306309
* Creates a new Form which you can
307310
* Accepts partial options to override any options passed to constructor.
308311
*/
309-
async createForm(optionOverrides: OverrideFeedbackConfiguration = {}): Promise<FeedbackDialog> {
312+
async createForm(
313+
optionOverrides: OverrideFeedbackConfiguration = {},
314+
): Promise<ReturnType<FeedbackModalIntegration['createDialog']>> {
310315
return _loadAndRenderDialog(mergeOptions(_options, optionOverrides));
311316
},
312317

‎packages/feedback/src/modal/components/Dialog.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { FeedbackFormData, FeedbackInternalOptions } from '@sentry/types';
33
import { Fragment, h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
44
import type { VNode } from 'preact';
55
import { useCallback, useMemo, useState } from 'preact/hooks';
6+
67
import { SUCCESS_MESSAGE_TIMEOUT } from '../../constants';
78
import { DialogHeader } from './DialogHeader';
89
import type { Props as HeaderProps } from './DialogHeader';

‎packages/feedback/src/modal/integration.tsx‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,7 @@
11
import { getCurrentScope, getGlobalScope, getIsolationScope } from '@sentry/core';
2-
import type {
3-
CreateDialogProps,
4-
FeedbackDialog,
5-
FeedbackFormData,
6-
FeedbackModalIntegration,
7-
IntegrationFn,
8-
User,
9-
} from '@sentry/types';
2+
import type { FeedbackFormData, FeedbackModalIntegration, IntegrationFn, User } from '@sentry/types';
103
import { h, render } from 'preact';
4+
import * as hooks from 'preact/hooks';
115
import { DOCUMENT } from '../constants';
126
import { Dialog } from './components/Dialog';
137
import { createDialogStyles } from './components/Dialog.css';
@@ -30,7 +24,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
3024
name: 'FeedbackModal',
3125
// eslint-disable-next-line @typescript-eslint/no-empty-function
3226
setupOnce() {},
33-
createDialog: ({ options, screenshotIntegration, sendFeedback, shadow }: CreateDialogProps) => {
27+
createDialog: ({ options, screenshotIntegration, sendFeedback, shadow }) => {
3428
const shadowRoot = shadow as unknown as ShadowRoot;
3529
const userKey = options.useSentryUser;
3630
const user = getUser();
@@ -39,7 +33,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
3933
const style = createDialogStyles();
4034

4135
let originalOverflow = '';
42-
const dialog: FeedbackDialog = {
36+
const dialog: ReturnType<FeedbackModalIntegration['createDialog']> = {
4337
get el() {
4438
return el;
4539
},
@@ -66,7 +60,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
6660
},
6761
};
6862

69-
const screenshotInput = screenshotIntegration && screenshotIntegration.createInput(h, dialog, options);
63+
const screenshotInput = screenshotIntegration && screenshotIntegration.createInput({ h, hooks, dialog, options });
7064

7165
const renderContent = (open: boolean): void => {
7266
render(

‎packages/feedback/src/screenshot/components/ScreenshotEditor.tsx‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
1-
import type { FeedbackDialog, FeedbackInternalOptions } from '@sentry/types';
21
/* eslint-disable max-lines */
2+
import type { FeedbackInternalOptions, FeedbackModalIntegration } from '@sentry/types';
33
import type { ComponentType, VNode, h as hType } from 'preact';
4-
// biome-ignore lint: needed for preact
5-
import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
6-
import { useCallback, useEffect, useMemo, useRef, useState } from 'preact/hooks';
4+
import type * as Hooks from 'preact/hooks';
75
import { DOCUMENT, WINDOW } from '../../constants';
86
import { createScreenshotInputStyles } from './ScreenshotInput.css';
9-
import { useTakeScreenshot } from './useTakeScreenshot';
7+
import { useTakeScreenshotFactory } from './useTakeScreenshot';
108

119
const CROP_BUTTON_SIZE = 30;
1210
const CROP_BUTTON_BORDER = 3;
@@ -15,8 +13,9 @@ const DPI = WINDOW.devicePixelRatio;
1513

1614
interface FactoryParams {
1715
h: typeof hType;
16+
hooks: typeof Hooks;
1817
imageBuffer: HTMLCanvasElement;
19-
dialog: FeedbackDialog;
18+
dialog: ReturnType<FeedbackModalIntegration['createDialog']>;
2019
options: FeedbackInternalOptions;
2120
}
2221

@@ -62,17 +61,25 @@ const getContainedSize = (img: HTMLCanvasElement): Box => {
6261
return { startX: x, startY: y, endX: width + x, endY: height + y };
6362
};
6463

65-
export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: FactoryParams): ComponentType<Props> {
64+
export function ScreenshotEditorFactory({
65+
h, // eslint-disable-line @typescript-eslint/no-unused-vars
66+
hooks,
67+
imageBuffer,
68+
dialog,
69+
options,
70+
}: FactoryParams): ComponentType<Props> {
71+
const useTakeScreenshot = useTakeScreenshotFactory({ hooks });
72+
6673
return function ScreenshotEditor({ onError }: Props): VNode {
67-
const styles = useMemo(() => ({ __html: createScreenshotInputStyles().innerText }), []);
74+
const styles = hooks.useMemo(() => ({ __html: createScreenshotInputStyles().innerText }), []);
6875

69-
const canvasContainerRef = useRef<HTMLDivElement>(null);
70-
const cropContainerRef = useRef<HTMLDivElement>(null);
71-
const croppingRef = useRef<HTMLCanvasElement>(null);
72-
const [croppingRect, setCroppingRect] = useState<Box>({ startX: 0, startY: 0, endX: 0, endY: 0 });
73-
const [confirmCrop, setConfirmCrop] = useState(false);
76+
const canvasContainerRef = hooks.useRef<HTMLDivElement>(null);
77+
const cropContainerRef = hooks.useRef<HTMLDivElement>(null);
78+
const croppingRef = hooks.useRef<HTMLCanvasElement>(null);
79+
const [croppingRect, setCroppingRect] = hooks.useState<Box>({ startX: 0, startY: 0, endX: 0, endY: 0 });
80+
const [confirmCrop, setConfirmCrop] = hooks.useState(false);
7481

75-
useEffect(() => {
82+
hooks.useEffect(() => {
7683
WINDOW.addEventListener('resize', resizeCropper, false);
7784
}, []);
7885

@@ -99,7 +106,7 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
99106
setCroppingRect({ startX: 0, startY: 0, endX: imageDimensions.width, endY: imageDimensions.height });
100107
}
101108

102-
useEffect(() => {
109+
hooks.useEffect(() => {
103110
const cropper = croppingRef.current;
104111
if (!cropper) {
105112
return;
@@ -141,7 +148,7 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
141148
DOCUMENT.addEventListener('mousemove', handleMouseMove);
142149
}
143150

144-
const makeHandleMouseMove = useCallback((corner: string) => {
151+
const makeHandleMouseMove = hooks.useCallback((corner: string) => {
145152
return function (e: MouseEvent) {
146153
if (!croppingRef.current) {
147154
return;
@@ -218,10 +225,10 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
218225
}
219226

220227
useTakeScreenshot({
221-
onBeforeScreenshot: useCallback(() => {
228+
onBeforeScreenshot: hooks.useCallback(() => {
222229
(dialog.el as HTMLElement).style.display = 'none';
223230
}, []),
224-
onScreenshot: useCallback(
231+
onScreenshot: hooks.useCallback(
225232
(imageSource: HTMLVideoElement) => {
226233
const context = imageBuffer.getContext('2d');
227234
if (!context) {
@@ -235,13 +242,13 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
235242
},
236243
[imageBuffer],
237244
),
238-
onAfterScreenshot: useCallback(() => {
245+
onAfterScreenshot: hooks.useCallback(() => {
239246
(dialog.el as HTMLElement).style.display = 'block';
240247
const container = canvasContainerRef.current;
241248
container && container.appendChild(imageBuffer);
242249
resizeCropper();
243250
}, []),
244-
onError: useCallback(error => {
251+
onError: hooks.useCallback(error => {
245252
(dialog.el as HTMLElement).style.display = 'block';
246253
onError(error);
247254
}, []),

‎packages/feedback/src/screenshot/components/useTakeScreenshot.tsx‎

Lines changed: 40 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,51 @@
1-
// biome-ignore lint/nursery/noUnusedImports: reason
2-
import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
3-
import { useEffect } from 'preact/hooks';
1+
import type * as Hooks from 'preact/hooks';
42
import { DOCUMENT, NAVIGATOR, WINDOW } from '../../constants';
53

4+
interface FactoryParams {
5+
hooks: typeof Hooks;
6+
}
7+
68
interface Props {
79
onBeforeScreenshot: () => void;
810
onScreenshot: (imageSource: HTMLVideoElement) => void;
911
onAfterScreenshot: () => void;
1012
onError: (error: Error) => void;
1113
}
1214

13-
export const useTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props): void => {
14-
useEffect(() => {
15-
const takeScreenshot = async (): Promise<void> => {
16-
onBeforeScreenshot();
17-
const stream = await NAVIGATOR.mediaDevices.getDisplayMedia({
18-
video: {
19-
width: WINDOW.innerWidth * WINDOW.devicePixelRatio,
20-
height: WINDOW.innerHeight * WINDOW.devicePixelRatio,
21-
},
22-
audio: false,
23-
// @ts-expect-error experimental flags: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia#prefercurrenttab
24-
monitorTypeSurfaces: 'exclude',
25-
preferCurrentTab: true,
26-
selfBrowserSurface: 'include',
27-
surfaceSwitching: 'exclude',
28-
});
15+
type UseTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) => void;
2916

30-
const video = DOCUMENT.createElement('video');
31-
await new Promise<void>((resolve, reject) => {
32-
video.srcObject = stream;
33-
video.onloadedmetadata = () => {
34-
onScreenshot(video);
35-
stream.getTracks().forEach(track => track.stop());
36-
resolve();
37-
};
38-
video.play().catch(reject);
39-
});
40-
onAfterScreenshot();
41-
};
17+
export function useTakeScreenshotFactory({ hooks }: FactoryParams): UseTakeScreenshot {
18+
return function useTakeScreenshot({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) {
19+
hooks.useEffect(() => {
20+
const takeScreenshot = async (): Promise<void> => {
21+
onBeforeScreenshot();
22+
const stream = await NAVIGATOR.mediaDevices.getDisplayMedia({
23+
video: {
24+
width: WINDOW.innerWidth * WINDOW.devicePixelRatio,
25+
height: WINDOW.innerHeight * WINDOW.devicePixelRatio,
26+
},
27+
audio: false,
28+
// @ts-expect-error experimental flags: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia#prefercurrenttab
29+
monitorTypeSurfaces: 'exclude',
30+
preferCurrentTab: true,
31+
selfBrowserSurface: 'include',
32+
surfaceSwitching: 'exclude',
33+
});
4234

43-
takeScreenshot().catch(onError);
44-
}, []);
45-
};
35+
const video = DOCUMENT.createElement('video');
36+
await new Promise<void>((resolve, reject) => {
37+
video.srcObject = stream;
38+
video.onloadedmetadata = () => {
39+
onScreenshot(video);
40+
stream.getTracks().forEach(track => track.stop());
41+
resolve();
42+
};
43+
video.play().catch(reject);
44+
});
45+
onAfterScreenshot();
46+
};
47+
48+
takeScreenshot().catch(onError);
49+
}, []);
50+
};
51+
}

0 commit comments

Comments
 (0)