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.

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(ui): Remove lazy renderer component prop #69823

Merged
merged 6 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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