Skip to content

Commit

Permalink
fix(Forms): console log a warning when Value.SummaryList gets an inva…
Browse files Browse the repository at this point in the history
…lid child (#4249)

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.
  • Loading branch information
tujoworker authored Nov 11, 2024
1 parent 505dbc1 commit 011e9eb
Show file tree
Hide file tree
Showing 6 changed files with 252 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<DlAllProps, 'label'> & {
export type Props = Omit<DlAllProps, 'label' | 'children'> & {
children: React.ReactNode
transformLabel?: ValueProps['transformLabel']
inheritVisibility?: ValueProps['inheritVisibility']
inheritLabel?: ValueProps['inheritLabel']
Expand All @@ -29,8 +31,14 @@ function SummaryList(props: Props) {
inheritLabel,
})

const { verifyChild } = useVerifyChildren({
children,
message: 'Value.SummaryList accepts only Value.* components!',
ignoreTypes: ['ValueBlock'],
})

return (
<SummaryListContext.Provider value={{ layout }}>
<SummaryListContext.Provider value={{ layout, verifyChild }}>
<Dl
className={classnames('dnb-forms-summary-list', className)}
layout={layout}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { DlProps } from '../../../../elements/Dl'
export type SummaryListContextProps = {
layout?: DlProps['layout']
isNested?: boolean
verifyChild?: () => void
}

const SummaryListContext = React.createContext<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<SummaryList>
<Form.SubHeading>Heading</Form.SubHeading>
<Value.String label="Label" value="Value" />
</SummaryList>
)

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(
<SummaryList>
<>
<Form.SubHeading>Heading</Form.SubHeading>
<Value.String label="Label" value="Value" />
</>
</SummaryList>
)

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(
<SummaryList top="x-large">Space Summary</SummaryList>
Expand Down
Original file line number Diff line number Diff line change
@@ -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 <button onClick={verifyChild}>Verify</button>
}

render(<TestComponent />)

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: (
<>
<span>Child 1</span>
<span>Child 2</span>
</>
),
message: 'Warning message',
})
return <button onClick={() => verifyChild()}>Verify</button>
}

const { getByText } = render(<TestComponent />)
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: (
<>
<IgnoredComponent />
<span>Child</span>
</>
),
message: 'Warning message',
ignoreTypes: ['IgnoredComponent'],
})

verifyChild() // Call verifyChild once

return <button onClick={() => verifyChild()}>Verify</button>
}

render(<TestComponent />)

expect(log).not.toHaveBeenCalled()

log.mockRestore()
})
})

describe('countChildren', () => {
it('should count valid children elements', () => {
const children = (
<>
<span>Child 1</span>
{null}
<span>Child 2</span>
</>
)

const count = countChildren(children)
expect(count).toBe(2)
})

it('should not count fragments or ignored types', () => {
const children = (
<>
<span>Child 1</span>
<React.Fragment>
<span>Child 2</span>
</React.Fragment>
<span>Child 3</span>
</>
)

const count = countChildren(children, ['span'])
expect(count).toBe(3)
})

it('should handle deeply nested structures', () => {
const children = (
<>
<span>Child 1</span>
<>
<span>Child 2</span>
<>
<span>Child 3</span>
</>
</>
</>
)

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)
})
})
Original file line number Diff line number Diff line change
@@ -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<string>
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<string>,
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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value> = ValueProps<Value>

Expand All @@ -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,
Expand Down

0 comments on commit 011e9eb

Please sign in to comment.