Skip to content

Commit

Permalink
Used shallowequal to compare props in usePopup
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexShukel committed Jan 22, 2024
1 parent 116b081 commit 2d7ff3f
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 11 deletions.
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"editor.codeActionsOnSave": {
"source.fixAll": true
"source.fixAll": "explicit"
}
}
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@testing-library/react": "^13.3.0",
"@types/jest": "^29.5.1",
"@types/react": "^18.0.9",
"@types/shallowequal": "^1.1.5",
"@typescript-eslint/eslint-plugin": "^4.33.0",
"aqu": "^0.4.3",
"clean-publish": "^4.2.0",
Expand All @@ -56,13 +57,14 @@
"dependencies": {
"@sirse-dev/safe-context": "^0.3.0",
"nanoid": "^3.3.6",
"shallowequal": "^1.1.0",
"tiny-invariant": "^1.2.0"
},
"peerDependencies": {
"react": ">=16"
},
"packageManager": "[email protected]",
"engines": {
"node": "18.12.1"
"node": "20.11.0"
}
}
14 changes: 14 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 8 additions & 1 deletion src/hooks/usePopup.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ComponentType, useCallback, useEffect, useRef } from 'react';
import { nanoid } from 'nanoid/non-secure';
import shallowEqual from 'shallowequal';

import { useEvent } from './useEvent';
import { usePopupsContext } from './usePopupsContext';
Expand Down Expand Up @@ -46,8 +47,14 @@ export const usePopup = <P, K extends keyof P>(
closePopup(popupIdentifier.current);
}, [closePopup]);

const oldPropsRef = useRef(props);

useEffect(() => {
update(popupIdentifier.current, props);
if (!shallowEqual(props, oldPropsRef.current)) {
update(popupIdentifier.current, props);
}

oldPropsRef.current = props;
}, [props, update]);

return [open, close];
Expand Down
105 changes: 97 additions & 8 deletions test/hooks/usePopup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { act, renderHook, screen } from '@testing-library/react';
import { group, TestHookWrapper } from './TestHookWrapper';
import { useCloseHandler } from '../../src/hooks/useCloseHandler';
import { usePopup } from '../../src/hooks/usePopup';
import { PopupsContext } from '../../src/PopupsContext';
import { PopupsBag } from '../../src/types/PopupsBag';

const SimplePopupComponent: React.FC = jest.fn(() => {
const unmount = useCloseHandler(() => {
Expand All @@ -17,16 +19,32 @@ const CustomizablePopupComponent: React.FC<{ message: string }> = jest.fn(
({ message }) => <div>{message}</div>
);

type ComplexPopupComponentProps = {
a: { value: string };
b: string;
};

const ComplexPopupComponent: React.FC<ComplexPopupComponentProps> = ({
a,
b,
}) => {
return (
<div>
<span>{a.value}</span>
<span>{b}</span>
</div>
);
};

const PopupComponentWithProps: React.FC<{
prop1: number;
prop2: string;
}> = jest.fn(() => <div>popup with props</div>);

describe('usePopup', () => {
it('should render only one popup', () => {
const initialProps = {};
const { result } = renderHook(
() => usePopup(SimplePopupComponent, initialProps, group),
() => usePopup(SimplePopupComponent, {}, group),
{
wrapper: TestHookWrapper,
}
Expand All @@ -46,9 +64,8 @@ describe('usePopup', () => {
});

it('should close popup', () => {
const initialProps = {};
const { result } = renderHook(
() => usePopup(SimplePopupComponent, initialProps, group),
() => usePopup(SimplePopupComponent, {}, group),
{
wrapper: TestHookWrapper,
}
Expand All @@ -71,9 +88,8 @@ describe('usePopup', () => {
const initialMessage = 'initial message';
const updatedMessage = 'updated message';

const initialProps = {};
const { result } = renderHook(
() => usePopup(CustomizablePopupComponent, initialProps, group),
() => usePopup(CustomizablePopupComponent, {}, group),
{ wrapper: TestHookWrapper }
);

Expand All @@ -93,9 +109,8 @@ describe('usePopup', () => {
});

it('should merge props', () => {
const initialProps = { prop1: 42 };
const { result } = renderHook(
() => usePopup(PopupComponentWithProps, initialProps, group),
() => usePopup(PopupComponentWithProps, { prop1: 42 }, group),
{
wrapper: TestHookWrapper,
}
Expand Down Expand Up @@ -137,4 +152,78 @@ describe('usePopup', () => {

expect(screen.getByText('updated')).toBeDefined();
});

it.only('should make shallow copy on values in props when updating popup', () => {
const initialA = {
value: 'A',
};

const mount = jest.fn();
const unmount = jest.fn();
const close = jest.fn();
const update = jest.fn();

const { result, rerender } = renderHook(
(props: ComplexPopupComponentProps) =>
usePopup(ComplexPopupComponent, props, group),
{
wrapper: ({ children }) => (
<PopupsContext.Provider
value={
{
mount,
unmount,
close,
update,
} as unknown as PopupsBag
}
>
{children}
</PopupsContext.Provider>
),
initialProps: {
a: initialA,
b: 'B',
},
}
);

act(() => {
result.current[0]();
});

expect(mount).toBeCalled();

rerender({
a: initialA,
b: 'B',
});

expect(update).toBeCalledTimes(0);

rerender({
a: initialA,
b: 'C',
});

expect(update).toBeCalledTimes(1);

rerender({
a: initialA,
b: 'C',
c: 'HELLO',
} as unknown as ComplexPopupComponentProps);

expect(update).toBeCalledTimes(2);

rerender({
a: {
value: 'A',
},
b: 'C',
c: 'HELLO',
} as unknown as ComplexPopupComponentProps);

expect(update).toBeCalledTimes(3);
});
});

0 comments on commit 2d7ff3f

Please sign in to comment.