Skip to content

Commit

Permalink
fix(feedback): Inject preact from feedbackModal into feedbackScreensh…
Browse files Browse the repository at this point in the history
…ot 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
  • Loading branch information
ryan953 authored Jun 24, 2024
1 parent 70e942d commit c49c9f3
Show file tree
Hide file tree
Showing 9 changed files with 129 additions and 112 deletions.
18 changes: 9 additions & 9 deletions docs/migration/feedback.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ Below you can find a list of relevant feedback changes and issues that have been
We have streamlined the interface for interacting with the Feedback widget. Below is a list of public functions that
existed in 7.x and a description of how they have changed in v8.

| Method Name | Replacement | Notes |
| ------------------------------------------------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `Sentry.getClient<BrowserClient>()?.getIntegration(Feedback)` | `const feedback = Sentry.getFeedback()` | Get a type-safe reference to the configured feedbackIntegration instance. |
| `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. |
| `feedback.openDialog()` | `widget.open()` | Make the form inside the widget visible. |
| `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. |
| `feedback.removeWidget()` | `widget.removeFromDom()` | Remove the form and widget instance from the page. After calling this `widget.el.parentNode` will be set to null. |
| `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. |
| - | `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. |
| Method Name | Replacement | Notes |
| ------------------------------------------------------------- | -------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `Sentry.getClient<BrowserClient>()?.getIntegration(Feedback)` | `const feedback = Sentry.getFeedback()` | Get a type-safe reference to the configured feedbackIntegration instance. |
| `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. |
| `feedback.openDialog()` | `widget.open()` | Make the form inside the widget visible. |
| `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. |
| `feedback.removeWidget()` | `widget.removeFromDom()` | Remove the form and widget instance from the page. After calling this `widget.el.parentNode` will be set to null. |
| `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. |
| - | `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. |

### API Examples

Expand Down
15 changes: 10 additions & 5 deletions packages/feedback/src/core/integration.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { getClient } from '@sentry/core';
import type {
FeedbackDialog,
FeedbackInternalOptions,
FeedbackModalIntegration,
FeedbackScreenshotIntegration,
Expand Down Expand Up @@ -56,7 +55,9 @@ export const buildFeedbackIntegration = ({
}: BuilderOptions): IntegrationFn<
Integration & {
attachTo(el: Element | string, optionOverrides?: OverrideFeedbackConfiguration): Unsubscribe;
createForm(optionOverrides?: OverrideFeedbackConfiguration): Promise<FeedbackDialog>;
createForm(
optionOverrides?: OverrideFeedbackConfiguration,
): Promise<ReturnType<FeedbackModalIntegration['createDialog']>>;
createWidget(optionOverrides?: OverrideFeedbackConfiguration): ActorComponent;
remove(): void;
}
Expand Down Expand Up @@ -179,7 +180,9 @@ export const buildFeedbackIntegration = ({
return integration as I;
};

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

let dialog: FeedbackDialog | null = null;
let dialog: ReturnType<FeedbackModalIntegration['createDialog']> | null = null;
const handleClick = async (): Promise<void> => {
if (!dialog) {
dialog = await _loadAndRenderDialog({
Expand Down Expand Up @@ -306,7 +309,9 @@ export const buildFeedbackIntegration = ({
* Creates a new Form which you can
* Accepts partial options to override any options passed to constructor.
*/
async createForm(optionOverrides: OverrideFeedbackConfiguration = {}): Promise<FeedbackDialog> {
async createForm(
optionOverrides: OverrideFeedbackConfiguration = {},
): Promise<ReturnType<FeedbackModalIntegration['createDialog']>> {
return _loadAndRenderDialog(mergeOptions(_options, optionOverrides));
},

Expand Down
1 change: 1 addition & 0 deletions packages/feedback/src/modal/components/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { FeedbackFormData, FeedbackInternalOptions } from '@sentry/types';
import { Fragment, h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
import type { VNode } from 'preact';
import { useCallback, useMemo, useState } from 'preact/hooks';

import { SUCCESS_MESSAGE_TIMEOUT } from '../../constants';
import { DialogHeader } from './DialogHeader';
import type { Props as HeaderProps } from './DialogHeader';
Expand Down
16 changes: 5 additions & 11 deletions packages/feedback/src/modal/integration.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
import { getCurrentScope, getGlobalScope, getIsolationScope } from '@sentry/core';
import type {
CreateDialogProps,
FeedbackDialog,
FeedbackFormData,
FeedbackModalIntegration,
IntegrationFn,
User,
} from '@sentry/types';
import type { FeedbackFormData, FeedbackModalIntegration, IntegrationFn, User } from '@sentry/types';
import { h, render } from 'preact';
import * as hooks from 'preact/hooks';
import { DOCUMENT } from '../constants';
import { Dialog } from './components/Dialog';
import { createDialogStyles } from './components/Dialog.css';
Expand All @@ -30,7 +24,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
name: 'FeedbackModal',
// eslint-disable-next-line @typescript-eslint/no-empty-function
setupOnce() {},
createDialog: ({ options, screenshotIntegration, sendFeedback, shadow }: CreateDialogProps) => {
createDialog: ({ options, screenshotIntegration, sendFeedback, shadow }) => {
const shadowRoot = shadow as unknown as ShadowRoot;
const userKey = options.useSentryUser;
const user = getUser();
Expand All @@ -39,7 +33,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
const style = createDialogStyles();

let originalOverflow = '';
const dialog: FeedbackDialog = {
const dialog: ReturnType<FeedbackModalIntegration['createDialog']> = {
get el() {
return el;
},
Expand All @@ -66,7 +60,7 @@ export const feedbackModalIntegration = ((): FeedbackModalIntegration => {
},
};

const screenshotInput = screenshotIntegration && screenshotIntegration.createInput(h, dialog, options);
const screenshotInput = screenshotIntegration && screenshotIntegration.createInput({ h, hooks, dialog, options });

const renderContent = (open: boolean): void => {
render(
Expand Down
47 changes: 27 additions & 20 deletions packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import type { FeedbackDialog, FeedbackInternalOptions } from '@sentry/types';
/* eslint-disable max-lines */
import type { FeedbackInternalOptions, FeedbackModalIntegration } from '@sentry/types';
import type { ComponentType, VNode, h as hType } from 'preact';
// biome-ignore lint: needed for preact
import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
import { useCallback, useEffect, useMemo, useRef, useState } from 'preact/hooks';
import type * as Hooks from 'preact/hooks';
import { DOCUMENT, WINDOW } from '../../constants';
import { createScreenshotInputStyles } from './ScreenshotInput.css';
import { useTakeScreenshot } from './useTakeScreenshot';
import { useTakeScreenshotFactory } from './useTakeScreenshot';

const CROP_BUTTON_SIZE = 30;
const CROP_BUTTON_BORDER = 3;
Expand All @@ -15,8 +13,9 @@ const DPI = WINDOW.devicePixelRatio;

interface FactoryParams {
h: typeof hType;
hooks: typeof Hooks;
imageBuffer: HTMLCanvasElement;
dialog: FeedbackDialog;
dialog: ReturnType<FeedbackModalIntegration['createDialog']>;
options: FeedbackInternalOptions;
}

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

export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }: FactoryParams): ComponentType<Props> {
export function ScreenshotEditorFactory({
h, // eslint-disable-line @typescript-eslint/no-unused-vars
hooks,
imageBuffer,
dialog,
options,
}: FactoryParams): ComponentType<Props> {
const useTakeScreenshot = useTakeScreenshotFactory({ hooks });

return function ScreenshotEditor({ onError }: Props): VNode {
const styles = useMemo(() => ({ __html: createScreenshotInputStyles().innerText }), []);
const styles = hooks.useMemo(() => ({ __html: createScreenshotInputStyles().innerText }), []);

const canvasContainerRef = useRef<HTMLDivElement>(null);
const cropContainerRef = useRef<HTMLDivElement>(null);
const croppingRef = useRef<HTMLCanvasElement>(null);
const [croppingRect, setCroppingRect] = useState<Box>({ startX: 0, startY: 0, endX: 0, endY: 0 });
const [confirmCrop, setConfirmCrop] = useState(false);
const canvasContainerRef = hooks.useRef<HTMLDivElement>(null);
const cropContainerRef = hooks.useRef<HTMLDivElement>(null);
const croppingRef = hooks.useRef<HTMLCanvasElement>(null);
const [croppingRect, setCroppingRect] = hooks.useState<Box>({ startX: 0, startY: 0, endX: 0, endY: 0 });
const [confirmCrop, setConfirmCrop] = hooks.useState(false);

useEffect(() => {
hooks.useEffect(() => {
WINDOW.addEventListener('resize', resizeCropper, false);
}, []);

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

useEffect(() => {
hooks.useEffect(() => {
const cropper = croppingRef.current;
if (!cropper) {
return;
Expand Down Expand Up @@ -141,7 +148,7 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
DOCUMENT.addEventListener('mousemove', handleMouseMove);
}

const makeHandleMouseMove = useCallback((corner: string) => {
const makeHandleMouseMove = hooks.useCallback((corner: string) => {
return function (e: MouseEvent) {
if (!croppingRef.current) {
return;
Expand Down Expand Up @@ -218,10 +225,10 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
}

useTakeScreenshot({
onBeforeScreenshot: useCallback(() => {
onBeforeScreenshot: hooks.useCallback(() => {
(dialog.el as HTMLElement).style.display = 'none';
}, []),
onScreenshot: useCallback(
onScreenshot: hooks.useCallback(
(imageSource: HTMLVideoElement) => {
const context = imageBuffer.getContext('2d');
if (!context) {
Expand All @@ -235,13 +242,13 @@ export function makeScreenshotEditorComponent({ imageBuffer, dialog, options }:
},
[imageBuffer],
),
onAfterScreenshot: useCallback(() => {
onAfterScreenshot: hooks.useCallback(() => {
(dialog.el as HTMLElement).style.display = 'block';
const container = canvasContainerRef.current;
container && container.appendChild(imageBuffer);
resizeCropper();
}, []),
onError: useCallback(error => {
onError: hooks.useCallback(error => {
(dialog.el as HTMLElement).style.display = 'block';
onError(error);
}, []),
Expand Down
74 changes: 40 additions & 34 deletions packages/feedback/src/screenshot/components/useTakeScreenshot.tsx
Original file line number Diff line number Diff line change
@@ -1,45 +1,51 @@
// biome-ignore lint/nursery/noUnusedImports: reason
import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
import { useEffect } from 'preact/hooks';
import type * as Hooks from 'preact/hooks';
import { DOCUMENT, NAVIGATOR, WINDOW } from '../../constants';

interface FactoryParams {
hooks: typeof Hooks;
}

interface Props {
onBeforeScreenshot: () => void;
onScreenshot: (imageSource: HTMLVideoElement) => void;
onAfterScreenshot: () => void;
onError: (error: Error) => void;
}

export const useTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props): void => {
useEffect(() => {
const takeScreenshot = async (): Promise<void> => {
onBeforeScreenshot();
const stream = await NAVIGATOR.mediaDevices.getDisplayMedia({
video: {
width: WINDOW.innerWidth * WINDOW.devicePixelRatio,
height: WINDOW.innerHeight * WINDOW.devicePixelRatio,
},
audio: false,
// @ts-expect-error experimental flags: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia#prefercurrenttab
monitorTypeSurfaces: 'exclude',
preferCurrentTab: true,
selfBrowserSurface: 'include',
surfaceSwitching: 'exclude',
});
type UseTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) => void;

const video = DOCUMENT.createElement('video');
await new Promise<void>((resolve, reject) => {
video.srcObject = stream;
video.onloadedmetadata = () => {
onScreenshot(video);
stream.getTracks().forEach(track => track.stop());
resolve();
};
video.play().catch(reject);
});
onAfterScreenshot();
};
export function useTakeScreenshotFactory({ hooks }: FactoryParams): UseTakeScreenshot {
return function useTakeScreenshot({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) {
hooks.useEffect(() => {
const takeScreenshot = async (): Promise<void> => {
onBeforeScreenshot();
const stream = await NAVIGATOR.mediaDevices.getDisplayMedia({
video: {
width: WINDOW.innerWidth * WINDOW.devicePixelRatio,
height: WINDOW.innerHeight * WINDOW.devicePixelRatio,
},
audio: false,
// @ts-expect-error experimental flags: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getDisplayMedia#prefercurrenttab
monitorTypeSurfaces: 'exclude',
preferCurrentTab: true,
selfBrowserSurface: 'include',
surfaceSwitching: 'exclude',
});

takeScreenshot().catch(onError);
}, []);
};
const video = DOCUMENT.createElement('video');
await new Promise<void>((resolve, reject) => {
video.srcObject = stream;
video.onloadedmetadata = () => {
onScreenshot(video);
stream.getTracks().forEach(track => track.stop());
resolve();
};
video.play().catch(reject);
});
onAfterScreenshot();
};

takeScreenshot().catch(onError);
}, []);
};
}
Loading

0 comments on commit c49c9f3

Please sign in to comment.