From 1355ef98099df08ca77c84bb551d07d6fc617e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksandras=20=C5=A0ukelovi=C4=8D?= <64651944+AlexShukel@users.noreply.github.com> Date: Mon, 22 Jan 2024 14:43:52 +0200 Subject: [PATCH] Used shallowequal to compare props in usePopup (#88) * Used shallowequal to compare props in usePopup * Removed dependency array * Updated version to 0.5.1 --- .vscode/settings.json | 2 +- package.json | 6 +- pnpm-lock.yaml | 14 +++++ src/hooks/usePopup.ts | 11 +++- test/hooks/usePopup.test.tsx | 105 ++++++++++++++++++++++++++++++++--- 5 files changed, 125 insertions(+), 13 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 100f337..ed9fcfe 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,5 +1,5 @@ { "editor.codeActionsOnSave": { - "source.fixAll": true + "source.fixAll": "explicit" } } diff --git a/package.json b/package.json index c834ae1..663ace6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "reactive-popups", - "version": "0.5.0", + "version": "0.5.1", "description": "Easy popup management with React", "files": [ "dist" @@ -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", @@ -56,6 +57,7 @@ "dependencies": { "@sirse-dev/safe-context": "^0.3.0", "nanoid": "^3.3.6", + "shallowequal": "^1.1.0", "tiny-invariant": "^1.2.0" }, "peerDependencies": { @@ -63,6 +65,6 @@ }, "packageManager": "pnpm@8.6.9", "engines": { - "node": "18.12.1" + "node": "20.11.0" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ff9c38d..3ad85ed 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -11,6 +11,9 @@ dependencies: nanoid: specifier: ^3.3.6 version: 3.3.6 + shallowequal: + specifier: ^1.1.0 + version: 1.1.0 tiny-invariant: specifier: ^1.2.0 version: 1.2.0 @@ -25,6 +28,9 @@ devDependencies: '@types/react': specifier: ^18.0.9 version: 18.0.9 + '@types/shallowequal': + specifier: ^1.1.5 + version: 1.1.5 '@typescript-eslint/eslint-plugin': specifier: ^4.33.0 version: 4.33.0(@typescript-eslint/parser@4.33.0)(eslint@7.32.0)(typescript@4.5.4) @@ -2093,6 +2099,10 @@ packages: resolution: {integrity: sha512-G8hZ6XJiHnuhQKR7ZmysCeJWE08o8T0AXtk5darsCaTVsYZhhgUrq53jizaR2FvsoeCwJhlmwTjkXBY5Pn/ZHw==} dev: true + /@types/shallowequal@1.1.5: + resolution: {integrity: sha512-8afr1hbNqvZ/FBMY2mcfkkbk7xhlTZN4lVCgQf55YdjUQpWLemmrcvcHg94vjw+ZVIfPa3UZz/sOE6CkaMlDnQ==} + dev: true + /@types/stack-utils@2.0.1: resolution: {integrity: sha512-Hl219/BT5fLAaz6NDkSuhzasy49dwQS/DSdu4MdggFB8zcXv7vflBI3xp7FEmkmdDkBUI2bPUNeMttp2knYdxw==} dev: true @@ -5970,6 +5980,10 @@ packages: kind-of: 6.0.3 dev: true + /shallowequal@1.1.0: + resolution: {integrity: sha512-y0m1JoUZSlPAjXVtPPW70aZWfIL/dSP7AFkRnniLCrK/8MDKog3TySTBmckD+RObVxH0v4Tox67+F14PdED2oQ==} + dev: false + /shebang-command@2.0.0: resolution: {integrity: sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==} engines: {node: '>=8'} diff --git a/src/hooks/usePopup.ts b/src/hooks/usePopup.ts index 46fe0ed..61596fd 100644 --- a/src/hooks/usePopup.ts +++ b/src/hooks/usePopup.ts @@ -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'; @@ -46,9 +47,15 @@ export const usePopup = ( closePopup(popupIdentifier.current); }, [closePopup]); + const oldPropsRef = useRef(props); + useEffect(() => { - update(popupIdentifier.current, props); - }, [props, update]); + if (!shallowEqual(props, oldPropsRef.current)) { + update(popupIdentifier.current, props); + } + + oldPropsRef.current = props; + }); return [open, close]; }; diff --git a/test/hooks/usePopup.test.tsx b/test/hooks/usePopup.test.tsx index 09d356d..07802cd 100644 --- a/test/hooks/usePopup.test.tsx +++ b/test/hooks/usePopup.test.tsx @@ -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(() => { @@ -17,6 +19,23 @@ const CustomizablePopupComponent: React.FC<{ message: string }> = jest.fn( ({ message }) =>
{message}
); +type ComplexPopupComponentProps = { + a: { value: string }; + b: string; +}; + +const ComplexPopupComponent: React.FC = ({ + a, + b, +}) => { + return ( +
+ {a.value} + {b} +
+ ); +}; + const PopupComponentWithProps: React.FC<{ prop1: number; prop2: string; @@ -24,9 +43,8 @@ const PopupComponentWithProps: React.FC<{ describe('usePopup', () => { it('should render only one popup', () => { - const initialProps = {}; const { result } = renderHook( - () => usePopup(SimplePopupComponent, initialProps, group), + () => usePopup(SimplePopupComponent, {}, group), { wrapper: TestHookWrapper, } @@ -46,9 +64,8 @@ describe('usePopup', () => { }); it('should close popup', () => { - const initialProps = {}; const { result } = renderHook( - () => usePopup(SimplePopupComponent, initialProps, group), + () => usePopup(SimplePopupComponent, {}, group), { wrapper: TestHookWrapper, } @@ -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 } ); @@ -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, } @@ -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 }) => ( + + {children} + + ), + 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); + }); });