Skip to content

Commit

Permalink
feat(ui): Remove lazy renderer component prop (#69823)
Browse files Browse the repository at this point in the history
  • Loading branch information
scttcper committed Apr 29, 2024
1 parent eab0711 commit 67726bb
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 50 deletions.
19 changes: 8 additions & 11 deletions static/app/components/events/eventReplay/index.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Fragment, useCallback} from 'react';
import {Fragment, lazy} from 'react';
import ReactLazyLoad from 'react-lazyload';
import styled from '@emotion/styled';

Expand Down Expand Up @@ -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,
Expand All @@ -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'
);
Expand All @@ -69,7 +66,7 @@ function EventReplayContent({
return (
<ErrorBoundary mini>
<LazyLoad
component={replayOnboardingPanel}
LazyComponent={ReplayOnboardingPanel}
platform={platform}
projectId={projectId}
/>
Expand Down Expand Up @@ -148,12 +145,12 @@ function EventReplayContent({
{hasReplayClipFeature ? (
<LazyLoad
{...commonProps}
component={replayClipPreview}
LazyComponent={ReplayClipPreview}
clipOffsets={REPLAY_CLIP_OFFSETS}
overlayContent={overlayContent}
/>
) : (
<LazyLoad {...commonProps} component={replayPreview} />
<LazyLoad {...commonProps} LazyComponent={ReplayPreview} />
)}
</ReactLazyLoad>
</ReplayGroupContextProvider>
Expand Down
22 changes: 11 additions & 11 deletions static/app/components/lazyLoad.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import {lazy} from 'react';

import {render, screen} from 'sentry-test/reactTestingLibrary';

import LazyLoad from 'sentry/components/lazyLoad';
Expand All @@ -15,7 +17,6 @@ function BarComponent({}: TestProps) {
}

type ResolvedComponent = {default: React.ComponentType<TestProps>};
type GetComponent = () => Promise<ResolvedComponent>;

describe('LazyLoad', function () {
afterEach(() => {
Expand All @@ -25,7 +26,7 @@ describe('LazyLoad', function () {
it('renders with a loading indicator when promise is not resolved yet', function () {
const importTest = new Promise<ResolvedComponent>(() => {});
const getComponent = () => importTest;
render(<LazyLoad component={getComponent} />);
render(<LazyLoad LazyComponent={lazy(getComponent)} />);

// Should be loading
expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();
Expand All @@ -37,7 +38,7 @@ describe('LazyLoad', function () {
doResolve = resolve;
});

render(<LazyLoad component={() => importFoo} />);
render(<LazyLoad LazyComponent={lazy(() => importFoo)} />);

// Should be loading
expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();
Expand All @@ -58,7 +59,7 @@ describe('LazyLoad', function () {
);

try {
render(<LazyLoad component={getComponent} />);
render(<LazyLoad LazyComponent={lazy(getComponent)} />);
} catch (err) {
// ignore
}
Expand All @@ -78,7 +79,7 @@ describe('LazyLoad', function () {
});

// First render Foo
const {rerender} = render(<LazyLoad component={() => importFoo} />);
const {rerender} = render(<LazyLoad LazyComponent={lazy(() => importFoo)} />);
expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();

// resolve with foo
Expand All @@ -90,22 +91,21 @@ describe('LazyLoad', function () {
doResolve = resolve;
});

rerender(<LazyLoad component={() => importBar} />);
rerender(<LazyLoad LazyComponent={lazy(() => importBar)} />);
expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();

// resolve with bar
doResolve!({default: BarComponent});
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<ResolvedComponent>(() => {})
);
rerender(<LazyLoad component={getComponent2} />);
const getComponent2 = jest.fn(() => new Promise<ResolvedComponent>(() => {}));
const LazyGet = lazy(getComponent2);
rerender(<LazyLoad LazyComponent={LazyGet} />);
expect(getComponent2).toHaveBeenCalledTimes(1);

// Does not refetch on other prop changes
rerender(<LazyLoad component={getComponent2} testProp />);
rerender(<LazyLoad LazyComponent={LazyGet} testProp />);
expect(getComponent2).toHaveBeenCalledTimes(1);
});
});
36 changes: 9 additions & 27 deletions static/app/components/lazyLoad.tsx
Original file line number Diff line number Diff line change
@@ -1,29 +1,19 @@
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';

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 PromisedImport<C> = Promise<{default: C}>;

type ComponentType = React.ComponentType<any>;

type Props<C extends ComponentType> = React.ComponentProps<C> & {
/**
* Wrap the component with lazy() before passing it to LazyLoad.
*/
LazyComponent?: React.LazyExoticComponent<C>;

type Props<C extends React.LazyExoticComponent<C>> = React.ComponentProps<C> & {
/**
* Accepts a function to trigger the import resolution of the component.
* @deprecated Use `LazyComponent` instead and keep lazy() calls out of the render path.
* Wrap the component with `lazy()` before passing it to LazyLoad.
* This should be declared outside of the render funciton.
*/
component?: () => PromisedImport<C>;
LazyComponent: C;

/**
* Override the default fallback component.
Expand All @@ -42,20 +32,11 @@ type Props<C extends ComponentType> = React.ComponentProps<C> & {
*
* <LazyLoad LazyComponent={LazyComponent} someComponentProps={...} />
*/
function LazyLoad<C extends ComponentType>({
component,
loadingFallback,
function LazyLoad<C extends React.LazyExoticComponent<any>>({
LazyComponent,
loadingFallback,
...props
}: Props<C>) {
const LazyLoadedComponent = useMemo(() => {
if (LazyComponent) {
return LazyComponent;
}

return lazy<C>(() => retryableImport(component));
}, [component, LazyComponent]);

return (
<ErrorBoundary>
<Suspense
Expand All @@ -67,7 +48,8 @@ function LazyLoad<C extends ComponentType>({
)
}
>
<LazyLoadedComponent {...(props as React.ComponentProps<C>)} />
{/* Props are strongly typed when passed in, but seem to conflict with LazyExoticComponent */}
<LazyComponent {...(props as any)} />
</Suspense>
</ErrorBoundary>
);
Expand Down
5 changes: 4 additions & 1 deletion static/app/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 67726bb

Please sign in to comment.