From 011e9ebdbc2a624840d67b2ea22e1a5228b0af98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Mon, 11 Nov 2024 10:34:10 +0100 Subject: [PATCH] fix(Forms): console log a warning when Value.SummaryList gets an invalid child (#4249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit More info [here](https://dnb-it.slack.com/archives/C07JEJVSHJR/p1730882739880279?thread_ts=1730737161.151109&cid=C07JEJVSHJR). It verifies that only allowed children are passed. It does this by comparing the amount of given children components vs the amount of the actual wanted components (verified via a context call). When there are more other children than the wanted ones, we show a warning ⚠️ I'm not sure if there are smarter ways to do that. But as of now, I think this may work. --- .../forms/Value/SummaryList/SummaryList.tsx | 12 +- .../Value/SummaryList/SummaryListContext.tsx | 1 + .../__tests__/SummaryList.test.tsx | 44 ++++++ .../__tests__/useVerifyChildren.test.tsx | 131 ++++++++++++++++++ .../Value/SummaryList/useVerifyChildren.ts | 61 ++++++++ .../extensions/forms/hooks/useValueProps.ts | 5 + 6 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/useVerifyChildren.test.tsx create mode 100644 packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/useVerifyChildren.ts diff --git a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/SummaryList.tsx b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/SummaryList.tsx index 4981ec6ba36..edf7dbddc7c 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/SummaryList.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/SummaryList.tsx @@ -5,8 +5,10 @@ import SummaryListContext from './SummaryListContext' import Dl, { DlAllProps } from '../../../../elements/Dl' import ValueProvider from '../Provider/ValueProvider' import { ValueProps } from '../../types' +import { useVerifyChildren } from './useVerifyChildren' -export type Props = Omit & { +export type Props = Omit & { + children: React.ReactNode transformLabel?: ValueProps['transformLabel'] inheritVisibility?: ValueProps['inheritVisibility'] inheritLabel?: ValueProps['inheritLabel'] @@ -29,8 +31,14 @@ function SummaryList(props: Props) { inheritLabel, }) + const { verifyChild } = useVerifyChildren({ + children, + message: 'Value.SummaryList accepts only Value.* components!', + ignoreTypes: ['ValueBlock'], + }) + return ( - +
void } const SummaryListContext = React.createContext< diff --git a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/SummaryList.test.tsx b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/SummaryList.test.tsx index 90de9a0c898..7e566c288f8 100644 --- a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/SummaryList.test.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/SummaryList.test.tsx @@ -17,6 +17,50 @@ describe('Field.SummaryList', () => { expect(element.getAttribute('aria-label')).toBe('Aria Label') }) + it('should warn when child is not a Value.* component', () => { + const log = jest.spyOn(console, 'log').mockImplementation() + + render( + + Heading + + + ) + + expect(log).toHaveBeenCalledTimes(1) + expect(log).toHaveBeenLastCalledWith( + expect.any(String), + expect.stringContaining( + 'Value.SummaryList accepts only Value.* components!' + ) + ) + + log.mockRestore() + }) + + it('should warn when child is not a Value.* component and is inside a Fragment', () => { + const log = jest.spyOn(console, 'log').mockImplementation() + + render( + + <> + Heading + + + + ) + + expect(log).toHaveBeenCalledTimes(1) + expect(log).toHaveBeenLastCalledWith( + expect.any(String), + expect.stringContaining( + 'Value.SummaryList accepts only Value.* components!' + ) + ) + + log.mockRestore() + }) + it('should support spacing props', () => { const { rerender } = render( Space Summary diff --git a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/useVerifyChildren.test.tsx b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/useVerifyChildren.test.tsx new file mode 100644 index 00000000000..aaf4665df93 --- /dev/null +++ b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/__tests__/useVerifyChildren.test.tsx @@ -0,0 +1,131 @@ +import React from 'react' +import { render, act } from '@testing-library/react' +import { useVerifyChildren, countChildren } from '../useVerifyChildren' + +describe('useVerifyChildren', () => { + it('should not warn when no children are provided', () => { + const log = jest.spyOn(console, 'log').mockImplementation() + + const TestComponent = () => { + const { verifyChild } = useVerifyChildren({ + children: null, + message: 'Warning message', + }) + return + } + + render() + + expect(log).not.toHaveBeenCalled() + + log.mockRestore() + }) + + it('should warn if verifyChild is not called enough times for children', () => { + const log = jest.spyOn(console, 'log').mockImplementation() + + const TestComponent = () => { + const { verifyChild } = useVerifyChildren({ + children: ( + <> + Child 1 + Child 2 + + ), + message: 'Warning message', + }) + return + } + + const { getByText } = render() + const button = getByText('Verify') + + act(() => button.click()) + + expect(log).toHaveBeenCalledWith(expect.any(String), 'Warning message') + + log.mockRestore() + }) + + it('should ignore specified types', () => { + const log = jest.spyOn(console, 'log').mockImplementation() + + const IgnoredComponent = () => <>Ignored + + const TestComponent = () => { + const { verifyChild } = useVerifyChildren({ + children: ( + <> + + Child + + ), + message: 'Warning message', + ignoreTypes: ['IgnoredComponent'], + }) + + verifyChild() // Call verifyChild once + + return + } + + render() + + expect(log).not.toHaveBeenCalled() + + log.mockRestore() + }) +}) + +describe('countChildren', () => { + it('should count valid children elements', () => { + const children = ( + <> + Child 1 + {null} + Child 2 + + ) + + const count = countChildren(children) + expect(count).toBe(2) + }) + + it('should not count fragments or ignored types', () => { + const children = ( + <> + Child 1 + + Child 2 + + Child 3 + + ) + + const count = countChildren(children, ['span']) + expect(count).toBe(3) + }) + + it('should handle deeply nested structures', () => { + const children = ( + <> + Child 1 + <> + Child 2 + <> + Child 3 + + + + ) + + const count = countChildren(children) + expect(count).toBe(3) + }) + + it('should return zero for primitive children', () => { + const children = 'Primitive Text Node' + const count = countChildren(children) + expect(count).toBe(0) + }) +}) diff --git a/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/useVerifyChildren.ts b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/useVerifyChildren.ts new file mode 100644 index 00000000000..5576f67ba63 --- /dev/null +++ b/packages/dnb-eufemia/src/extensions/forms/Value/SummaryList/useVerifyChildren.ts @@ -0,0 +1,61 @@ +import { + Children, + Fragment, + isValidElement, + useCallback, + useEffect, + useRef, +} from 'react' +import { warn } from '../../../../shared/helpers' + +export function useVerifyChildren({ + children, + message, + ignoreTypes, +}: { + ignoreTypes?: Array + children: React.ReactNode + message: string +}) { + const verifyCount = useRef(0) + verifyCount.current = 0 + const verifyChild = useCallback(() => { + verifyCount.current += 1 + }, []) + + useEffect(() => { + if (process.env.NODE_ENV !== 'production') { + const count = countChildren(children, ignoreTypes) + if (count > 0 && count > verifyCount.current) { + warn(message) + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [children, message]) + + return { verifyChild } +} + +/** + * Count the children of a React node, + * without counting React.Fragment or primitive nodes. + */ +export const countChildren = ( + children: React.ReactNode, + ignoreTypes?: Array, + count = 0 +) => { + return Children.toArray(children).reduce((count: number, child) => { + if (child?.['type'] === Fragment) { + return countChildren(child['props']?.children, ignoreTypes, count) + } + + return ( + count + + (isValidElement(child) && + !ignoreTypes?.includes(child?.type?.['name']) + ? 1 + : 0) + ) + }, count) +} diff --git a/packages/dnb-eufemia/src/extensions/forms/hooks/useValueProps.ts b/packages/dnb-eufemia/src/extensions/forms/hooks/useValueProps.ts index 873bc37cac0..c921241ca31 100644 --- a/packages/dnb-eufemia/src/extensions/forms/hooks/useValueProps.ts +++ b/packages/dnb-eufemia/src/extensions/forms/hooks/useValueProps.ts @@ -10,6 +10,7 @@ import useExternalValue from './useExternalValue' import usePath from './usePath' import DataContext from '../DataContext/Context' import ValueProviderContext from '../Value/Provider/ValueProviderContext' +import SummaryListContext from '../Value/SummaryList/SummaryListContext' export type Props = ValueProps @@ -22,6 +23,10 @@ export default function useValueProps< const { extend } = useContext(ValueProviderContext) const props = extend(localProps) + // Only to log a warning in the Value.SummaryList component + const { verifyChild } = useContext(SummaryListContext) || {} + verifyChild?.() + const { path: pathProp, value: valueProp,