From ae5f52642810b6fd632adea4dec8d18f15195f60 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Fri, 26 Apr 2024 15:42:20 -0700 Subject: [PATCH 1/5] feat(ui): Remove lazy renderer component prop final follow up to https://github.com/getsentry/sentry/pull/69740 prefer stable references to lazy() --- .../components/events/eventReplay/index.tsx | 19 +++++++--------- static/app/components/lazyLoad.spec.tsx | 22 +++++++++---------- static/app/components/lazyLoad.tsx | 10 +-------- 3 files changed, 20 insertions(+), 31 deletions(-) diff --git a/static/app/components/events/eventReplay/index.tsx b/static/app/components/events/eventReplay/index.tsx index e30afc944f1879..9094aac6a97787 100644 --- a/static/app/components/events/eventReplay/index.tsx +++ b/static/app/components/events/eventReplay/index.tsx @@ -1,4 +1,4 @@ -import {Fragment, useCallback} from 'react'; +import {Fragment, lazy} from 'react'; import ReactLazyLoad from 'react-lazyload'; import styled from '@emotion/styled'; @@ -36,6 +36,10 @@ export const REPLAY_CLIP_OFFSETS = { durationBeforeMs: 5_000, }; +const ReplayOnboardingPanel = lazy(() => import('./replayInlineOnboardingPanel')); +const ReplayPreview = lazy(() => import('./replayPreview')); +const ReplayClipPreview = lazy(() => import('./replayClipPreview')); + function EventReplayContent({ event, group, @@ -46,13 +50,6 @@ function EventReplayContent({ const router = useRouter(); const {getReplayCountForIssue} = useReplayCountForIssues(); - const replayOnboardingPanel = useCallback( - () => import('./replayInlineOnboardingPanel'), - [] - ); - const replayPreview = useCallback(() => import('./replayPreview'), []); - const replayClipPreview = useCallback(() => import('./replayClipPreview'), []); - const hasReplayClipFeature = organization.features.includes( 'issue-details-inline-replay-viewer' ); @@ -69,7 +66,7 @@ function EventReplayContent({ return ( @@ -148,12 +145,12 @@ function EventReplayContent({ {hasReplayClipFeature ? ( ) : ( - + )} diff --git a/static/app/components/lazyLoad.spec.tsx b/static/app/components/lazyLoad.spec.tsx index 5102597422b806..6915d87d407f3f 100644 --- a/static/app/components/lazyLoad.spec.tsx +++ b/static/app/components/lazyLoad.spec.tsx @@ -1,3 +1,5 @@ +import {lazy} from 'react'; + import {render, screen} from 'sentry-test/reactTestingLibrary'; import LazyLoad from 'sentry/components/lazyLoad'; @@ -15,7 +17,6 @@ function BarComponent({}: TestProps) { } type ResolvedComponent = {default: React.ComponentType}; -type GetComponent = () => Promise; describe('LazyLoad', function () { afterEach(() => { @@ -25,7 +26,7 @@ describe('LazyLoad', function () { it('renders with a loading indicator when promise is not resolved yet', function () { const importTest = new Promise(() => {}); const getComponent = () => importTest; - render(); + render(); // Should be loading expect(screen.getByTestId('loading-indicator')).toBeInTheDocument(); @@ -37,7 +38,7 @@ describe('LazyLoad', function () { doResolve = resolve; }); - render( importFoo} />); + render( importFoo)} />); // Should be loading expect(screen.getByTestId('loading-indicator')).toBeInTheDocument(); @@ -58,7 +59,7 @@ describe('LazyLoad', function () { ); try { - render(); + render(); } catch (err) { // ignore } @@ -78,7 +79,7 @@ describe('LazyLoad', function () { }); // First render Foo - const {rerender} = render( importFoo} />); + const {rerender} = render( importFoo)} />); expect(screen.getByTestId('loading-indicator')).toBeInTheDocument(); // resolve with foo @@ -90,7 +91,7 @@ describe('LazyLoad', function () { doResolve = resolve; }); - rerender( importBar} />); + rerender( importBar)} />); expect(screen.getByTestId('loading-indicator')).toBeInTheDocument(); // resolve with bar @@ -98,14 +99,13 @@ describe('LazyLoad', function () { expect(await screen.findByText('my bar component')).toBeInTheDocument(); // Update component prop to a mock to make sure it isn't re-called - const getComponent2: GetComponent = jest.fn( - () => new Promise(() => {}) - ); - rerender(); + const getComponent2 = jest.fn(() => new Promise(() => {})); + const LazyGet = lazy(getComponent2); + rerender(); expect(getComponent2).toHaveBeenCalledTimes(1); // Does not refetch on other prop changes - rerender(); + rerender(); expect(getComponent2).toHaveBeenCalledTimes(1); }); }); diff --git a/static/app/components/lazyLoad.tsx b/static/app/components/lazyLoad.tsx index 3ffc23fb8518b4..693c4413064f5f 100644 --- a/static/app/components/lazyLoad.tsx +++ b/static/app/components/lazyLoad.tsx @@ -9,21 +9,13 @@ import {t} from 'sentry/locale'; import {isWebpackChunkLoadingError} from 'sentry/utils'; import retryableImport from 'sentry/utils/retryableImport'; -type PromisedImport = Promise<{default: C}>; - type ComponentType = React.ComponentType; type Props = React.ComponentProps & { /** * Wrap the component with lazy() before passing it to LazyLoad. */ - LazyComponent?: React.LazyExoticComponent; - - /** - * Accepts a function to trigger the import resolution of the component. - * @deprecated Use `LazyComponent` instead and keep lazy() calls out of the render path. - */ - component?: () => PromisedImport; + LazyComponent: React.LazyExoticComponent; /** * Override the default fallback component. From 7921d976e0e894b72af89b925884b169bb176739 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Mon, 29 Apr 2024 10:04:05 -0700 Subject: [PATCH 2/5] remove usememo, type props stricter --- .../components/events/eventReplay/index.tsx | 2 +- static/app/components/lazyLoad.tsx | 24 +++++-------------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/static/app/components/events/eventReplay/index.tsx b/static/app/components/events/eventReplay/index.tsx index 9094aac6a97787..6325091dbb9c8a 100644 --- a/static/app/components/events/eventReplay/index.tsx +++ b/static/app/components/events/eventReplay/index.tsx @@ -66,7 +66,7 @@ function EventReplayContent({ return ( diff --git a/static/app/components/lazyLoad.tsx b/static/app/components/lazyLoad.tsx index 693c4413064f5f..92ac29dc16a526 100644 --- a/static/app/components/lazyLoad.tsx +++ b/static/app/components/lazyLoad.tsx @@ -1,5 +1,5 @@ import type {ErrorInfo} from 'react'; -import {Component, lazy, Suspense, useMemo} from 'react'; +import {Component, Suspense} from 'react'; import styled from '@emotion/styled'; import * as Sentry from '@sentry/react'; @@ -7,15 +7,12 @@ import LoadingError from 'sentry/components/loadingError'; import LoadingIndicator from 'sentry/components/loadingIndicator'; import {t} from 'sentry/locale'; import {isWebpackChunkLoadingError} from 'sentry/utils'; -import retryableImport from 'sentry/utils/retryableImport'; -type ComponentType = React.ComponentType; - -type Props = React.ComponentProps & { +type Props> = React.ComponentProps & { /** * Wrap the component with lazy() before passing it to LazyLoad. */ - LazyComponent: React.LazyExoticComponent; + LazyComponent: C; /** * Override the default fallback component. @@ -34,20 +31,11 @@ type Props = React.ComponentProps & { * * */ -function LazyLoad({ - component, - loadingFallback, +function LazyLoad>({ LazyComponent, + loadingFallback, ...props }: Props) { - const LazyLoadedComponent = useMemo(() => { - if (LazyComponent) { - return LazyComponent; - } - - return lazy(() => retryableImport(component)); - }, [component, LazyComponent]); - return ( ({ ) } > - )} /> + ); From 6b14e3111f5294eea5bd4e3be040be450cf3e249 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Mon, 29 Apr 2024 10:59:22 -0700 Subject: [PATCH 3/5] cast more types --- static/app/components/lazyLoad.tsx | 2 +- static/app/routes.tsx | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/static/app/components/lazyLoad.tsx b/static/app/components/lazyLoad.tsx index 92ac29dc16a526..549057d6dd7047 100644 --- a/static/app/components/lazyLoad.tsx +++ b/static/app/components/lazyLoad.tsx @@ -8,7 +8,7 @@ import LoadingIndicator from 'sentry/components/loadingIndicator'; import {t} from 'sentry/locale'; import {isWebpackChunkLoadingError} from 'sentry/utils'; -type Props> = React.ComponentProps & { +type Props> = React.ComponentProps & { /** * Wrap the component with lazy() before passing it to LazyLoad. */ diff --git a/static/app/routes.tsx b/static/app/routes.tsx index 10e424d974fdd3..bdee14bc21158a 100644 --- a/static/app/routes.tsx +++ b/static/app/routes.tsx @@ -28,7 +28,10 @@ import {IndexRoute, Route} from './components/route'; const hook = (name: HookName) => HookStore.get(name).map(cb => cb()); -const SafeLazyLoad = errorHandler(LazyLoad); +// LazyExoticComponent Props get crazy when wrapped in an additional layer +const SafeLazyLoad = errorHandler(LazyLoad) as unknown as React.ComponentType< + typeof LazyLoad +>; // NOTE: makeLazyloadComponent is exported for use in the sentry.io (getsentry) // pirvate routing tree. From 141723622786296dae29b8d169b032c3259dbb05 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Mon, 29 Apr 2024 11:00:49 -0700 Subject: [PATCH 4/5] add another comment --- static/app/components/lazyLoad.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/static/app/components/lazyLoad.tsx b/static/app/components/lazyLoad.tsx index 549057d6dd7047..b524989125257a 100644 --- a/static/app/components/lazyLoad.tsx +++ b/static/app/components/lazyLoad.tsx @@ -47,6 +47,7 @@ function LazyLoad>({ ) } > + {/* Props are strongly typed when passed in, but seem to conflict with LazyExoticComponent */} From a226922b60fa8120794ad4f48756ad16a1af9076 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Mon, 29 Apr 2024 11:14:42 -0700 Subject: [PATCH 5/5] add comment --- static/app/components/lazyLoad.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/static/app/components/lazyLoad.tsx b/static/app/components/lazyLoad.tsx index b524989125257a..84f0a41ab27e01 100644 --- a/static/app/components/lazyLoad.tsx +++ b/static/app/components/lazyLoad.tsx @@ -10,7 +10,8 @@ import {isWebpackChunkLoadingError} from 'sentry/utils'; type Props> = React.ComponentProps & { /** - * Wrap the component with lazy() before passing it to LazyLoad. + * Wrap the component with `lazy()` before passing it to LazyLoad. + * This should be declared outside of the render funciton. */ LazyComponent: C;