Skip to content

fix(react): Memoize wrapped component to prevent rerenders #17230

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

Merged
merged 2 commits into from
Jul 30, 2025
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-router/test/client/react-exports.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describe('Re-exports from React SDK', () => {
});

expect(WrappedComponent).toBeDefined();
expect(typeof WrappedComponent).toBe('function');
expect(typeof WrappedComponent).toBe('object');
expect(WrappedComponent.displayName).toBe('errorBoundary(TestComponent)');

const { getByText } = render(React.createElement(WrappedComponent));
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/errorboundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,11 @@ function withErrorBoundary<P extends Record<string, any>>(
): React.FC<P> {
const componentDisplayName = WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;

const Wrapped: React.FC<P> = (props: P) => (
const Wrapped = React.memo((props: P) => (
<ErrorBoundary {...errorBoundaryOptions}>
<WrappedComponent {...props} />
</ErrorBoundary>
);
)) as unknown as React.FC<P>;

Wrapped.displayName = `errorBoundary(${componentDisplayName})`;

Expand Down
116 changes: 116 additions & 0 deletions packages/react/test/errorboundary.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,122 @@ describe('withErrorBoundary', () => {
const Component = withErrorBoundary(() => <h1>Hello World</h1>, { fallback: <h1>fallback</h1> });
expect(Component.displayName).toBe(`errorBoundary(${UNKNOWN_COMPONENT})`);
});

it('does not rerender when props are identical', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I think these tests are useless, you're just testing react api here. I think we don't need tests for this at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave them in still, since they'll make sure we keep this memoized. But good point indeed

let renderCount = 0;
const TestComponent = ({ title }: { title: string }) => {
renderCount++;
return <h1>{title}</h1>;
};

const WrappedComponent = withErrorBoundary(TestComponent, { fallback: <h1>fallback</h1> });
const { rerender } = render(<WrappedComponent title="test" />);

expect(renderCount).toBe(1);

// Rerender with identical props - should not cause TestComponent to rerender
rerender(<WrappedComponent title="test" />);
expect(renderCount).toBe(1);

// Rerender with different props - should cause TestComponent to rerender
rerender(<WrappedComponent title="different" />);
expect(renderCount).toBe(2);
});

it('does not rerender when complex props are identical', () => {
let renderCount = 0;
const TestComponent = ({ data }: { data: { id: number; name: string } }) => {
renderCount++;
return <h1>{data.name}</h1>;
};

const WrappedComponent = withErrorBoundary(TestComponent, { fallback: <h1>fallback</h1> });
const props = { data: { id: 1, name: 'test' } };
const { rerender } = render(<WrappedComponent {...props} />);

expect(renderCount).toBe(1);

// Rerender with same object reference - should not cause TestComponent to rerender
rerender(<WrappedComponent {...props} />);
expect(renderCount).toBe(1);

// Rerender with different object but same values - should cause rerender
rerender(<WrappedComponent data={{ id: 1, name: 'test' }} />);
expect(renderCount).toBe(2);

// Rerender with different values - should cause rerender
rerender(<WrappedComponent data={{ id: 2, name: 'different' }} />);
expect(renderCount).toBe(3);
});

it('does not rerender when errorBoundaryOptions are the same', () => {
let renderCount = 0;
const TestComponent = ({ title }: { title: string }) => {
renderCount++;
return <h1>{title}</h1>;
};

const errorBoundaryOptions = { fallback: <h1>fallback</h1> };
const WrappedComponent = withErrorBoundary(TestComponent, errorBoundaryOptions);
const { rerender } = render(<WrappedComponent title="test" />);

expect(renderCount).toBe(1);

// Rerender with identical props - should not cause TestComponent to rerender
rerender(<WrappedComponent title="test" />);
expect(renderCount).toBe(1);
});

it('preserves function component behavior with React.memo', () => {
const TestComponent = ({ title }: { title: string }) => <h1>{title}</h1>;
const WrappedComponent = withErrorBoundary(TestComponent, { fallback: <h1>fallback</h1> });

expect(WrappedComponent).toBeDefined();
expect(typeof WrappedComponent).toBe('object');
expect(WrappedComponent.displayName).toBe('errorBoundary(TestComponent)');

const { container } = render(<WrappedComponent title="test" />);
expect(container.innerHTML).toContain('test');
});

it('does not rerender parent component unnecessarily', () => {
let parentRenderCount = 0;
let childRenderCount = 0;

const ChildComponent = ({ value }: { value: number }) => {
childRenderCount++;
return <div>Child: {value}</div>;
};

const WrappedChild = withErrorBoundary(ChildComponent, { fallback: <div>Error</div> });

const ParentComponent = ({ childValue, otherProp }: { childValue: number; otherProp: string }) => {
parentRenderCount++;
return (
<div>
<div>Parent: {otherProp}</div>
<WrappedChild value={childValue} />
</div>
);
};

const { rerender } = render(<ParentComponent childValue={1} otherProp="test" />);

expect(parentRenderCount).toBe(1);
expect(childRenderCount).toBe(1);

// Change otherProp but keep childValue the same
rerender(<ParentComponent childValue={1} otherProp="changed" />);

expect(parentRenderCount).toBe(2); // Parent should rerender
expect(childRenderCount).toBe(1); // Child should NOT rerender due to memo

// Change childValue
rerender(<ParentComponent childValue={2} otherProp="changed" />);

expect(parentRenderCount).toBe(3); // Parent should rerender
expect(childRenderCount).toBe(2); // Child should rerender due to changed props
});
});

describe('ErrorBoundary', () => {
Expand Down
Loading