From 0d332d0e76085df25a2d98337de2c7cdc40509ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 12 Feb 2025 11:59:32 +0100 Subject: [PATCH 1/5] [core] Make `mergeReactProps` work with non-native event handlers --- .../react/src/utils/mergeReactProps.test.ts | 37 ++++- packages/react/src/utils/mergeReactProps.ts | 132 +++++++++++------- 2 files changed, 112 insertions(+), 57 deletions(-) diff --git a/packages/react/src/utils/mergeReactProps.test.ts b/packages/react/src/utils/mergeReactProps.test.ts index fa7166711b..397c726c2f 100644 --- a/packages/react/src/utils/mergeReactProps.test.ts +++ b/packages/react/src/utils/mergeReactProps.test.ts @@ -15,9 +15,9 @@ describe('mergeReactProps', () => { }; const mergedProps = mergeReactProps<'button'>(theirProps, ourProps); - mergedProps.onClick?.({} as any); - mergedProps.onKeyDown?.({} as any); - mergedProps.onPaste?.({} as any); + mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); + mergedProps.onKeyDown?.({ nativeEvent: new MouseEvent('keydown') } as any); + mergedProps.onPaste?.({ nativeEvent: new Event('paste') } as any); expect(theirProps.onClick.calledBefore(ourProps.onClick)).to.equal(true); expect(theirProps.onClick.callCount).to.equal(1); @@ -47,7 +47,7 @@ describe('mergeReactProps', () => { }, ); - mergedProps.onClick?.({} as any); + mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); expect(log).to.deep.equal(['1', '2', '3']); }); @@ -106,7 +106,7 @@ describe('mergeReactProps', () => { }, ); - mergedProps.onClick?.({} as any); + mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); expect(ran).to.equal(true); }); @@ -132,7 +132,7 @@ describe('mergeReactProps', () => { }, ); - mergedProps.onClick?.({} as any); + mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); expect(ran).to.equal(false); }); @@ -159,11 +159,34 @@ describe('mergeReactProps', () => { }, ); - mergedProps.onClick?.({} as any); + mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') }); expect(log).to.deep.equal(['0', '1']); }); + [true, 13, 'newValue', { key: 'value' }, ['value'], () => 'value'].forEach((eventArgument) => { + it('handles non-standard event handlers without error', () => { + const log: string[] = []; + + const mergedProps = mergeReactProps( + { + onValueChange() { + log.push('0'); + }, + }, + { + onValueChange() { + log.push('1'); + }, + }, + ); + + mergedProps.onValueChange(eventArgument); + + expect(log).to.deep.equal(['0', '1']); + }); + }); + it('merges internal props so that the ones defined first override the ones defined later', () => { const mergedProps = mergeReactProps<'button'>( {}, diff --git a/packages/react/src/utils/mergeReactProps.ts b/packages/react/src/utils/mergeReactProps.ts index 62539d511f..910041963e 100644 --- a/packages/react/src/utils/mergeReactProps.ts +++ b/packages/react/src/utils/mergeReactProps.ts @@ -1,12 +1,12 @@ -import type * as React from 'react'; +import * as React from 'react'; import type { BaseUIEvent, WithBaseUIEvent } from './types'; /** * Merges multiple sets of React props such that their event handlers are called in sequence (the user's * before our internal ones), and allows the user to prevent the internal event handlers from being - * executed by attaching a `preventBaseUIHandler` method. It also merges the `style` prop, whereby - * the user's styles overwrite the internal ones. - * @important **`className` and `ref` are not merged.** + * executed by attaching a `preventBaseUIHandler` method. It also merges the `className` and `style` props, whereby + * the classes are concatenated and the user's styles overwrite the internal ones. + * @important **`ref` is not merged.** * @param externalProps the user's external props. * @param internalProps our own internal props. * @returns the merged props. @@ -32,56 +32,88 @@ function merge( } return Object.entries(externalProps).reduce( - (acc, [key, value]) => { - if ( - // This approach is more efficient than using a regex. - key[0] === 'o' && - key[1] === 'n' && - key.charCodeAt(2) >= 65 /* A */ && - key.charCodeAt(2) <= 90 /* Z */ && - typeof value === 'function' - ) { - acc[key] = (event: React.SyntheticEvent) => { - let isPrevented = false; - - const theirHandler = value; - const ourHandler = internalProps[key]; - - const baseUIEvent = event as BaseUIEvent; - - baseUIEvent.preventBaseUIHandler = () => { - isPrevented = true; - }; - - const result = theirHandler(baseUIEvent); - - if (!isPrevented) { - ourHandler?.(baseUIEvent); - } - - return result; - }; - } else if (key === 'style') { - if (value || internalProps.style) { - acc[key] = { ...internalProps.style, ...(value || {}) }; - } - } else if (key === 'className') { - if (value) { - if (internalProps.className) { - // eslint-disable-next-line prefer-template - acc[key] = value + ' ' + internalProps.className; - } else { - acc[key] = value; - } - } else { - acc[key] = internalProps.className; - } + (mergedProps, [externalPropName, externalPropValue]) => { + if (isEventHandler(externalPropName, externalPropValue)) { + mergedProps[externalPropName] = mergeEventHandlers( + externalPropValue, + internalProps[externalPropName], + ); + } else if (externalPropName === 'style') { + mergedProps[externalPropName] = mergeStyles( + externalPropValue as React.CSSProperties, + internalProps.style, + ); + } else if (externalPropName === 'className') { + mergeClassNames(externalPropValue as string, internalProps.className); } else { - acc[key] = value; + mergedProps[externalPropName] = externalPropValue; } - return acc; + return mergedProps; }, { ...internalProps }, ); } + +function isEventHandler(key: string, value: unknown) { + // This approach is more efficient than using a regex. + return ( + key[0] === 'o' && + key[1] === 'n' && + key.charCodeAt(2) >= 65 /* A */ && + key.charCodeAt(2) <= 90 /* Z */ && + typeof value === 'function' + ); +} + +function mergeEventHandlers(theirHandler: any, ourHandler: any) { + return (event: React.SyntheticEvent | {}) => { + if (isSyntheticEvent(event)) { + let isPrevented = false; + const baseUIEvent = event as BaseUIEvent; + + baseUIEvent.preventBaseUIHandler = () => { + isPrevented = true; + }; + + const result = theirHandler(baseUIEvent); + + if (!isPrevented) { + ourHandler?.(baseUIEvent); + } + + return result; + } + + const result = theirHandler(event); + ourHandler?.(event); + return result; + }; +} + +function mergeStyles( + theirStyle: React.CSSProperties | undefined, + ourStyle: React.CSSProperties | undefined, +) { + if (theirStyle || ourStyle) { + return { ...ourStyle, ...(theirStyle || {}) }; + } + return undefined; +} + +function mergeClassNames(theirClassName: string | undefined, ourClassName: string | undefined) { + if (theirClassName) { + if (ourClassName) { + // eslint-disable-next-line prefer-template + return theirClassName + ' ' + ourClassName; + } + + return theirClassName; + } + + return ourClassName; +} + +function isSyntheticEvent(event: React.SyntheticEvent | {}): event is React.SyntheticEvent { + return typeof event === 'object' && 'nativeEvent' in event; +} From 388feb3d45d341a45933139bb254de6f601f4a0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 12 Feb 2025 12:28:47 +0100 Subject: [PATCH 2/5] Fix className --- packages/react/src/utils/mergeReactProps.ts | 27 +++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/react/src/utils/mergeReactProps.ts b/packages/react/src/utils/mergeReactProps.ts index 910041963e..dd8aa95dbe 100644 --- a/packages/react/src/utils/mergeReactProps.ts +++ b/packages/react/src/utils/mergeReactProps.ts @@ -32,21 +32,21 @@ function merge( } return Object.entries(externalProps).reduce( - (mergedProps, [externalPropName, externalPropValue]) => { - if (isEventHandler(externalPropName, externalPropValue)) { - mergedProps[externalPropName] = mergeEventHandlers( - externalPropValue, - internalProps[externalPropName], - ); - } else if (externalPropName === 'style') { - mergedProps[externalPropName] = mergeStyles( + (mergedProps, [propName, externalPropValue]) => { + if (isEventHandler(propName, externalPropValue)) { + mergedProps[propName] = mergeEventHandlers(externalPropValue, internalProps[propName]); + } else if (propName === 'style') { + mergedProps[propName] = mergeStyles( externalPropValue as React.CSSProperties, internalProps.style, ); - } else if (externalPropName === 'className') { - mergeClassNames(externalPropValue as string, internalProps.className); + } else if (propName === 'className') { + mergedProps[propName] = mergeClassNames( + externalPropValue as string, + internalProps.className, + ); } else { - mergedProps[externalPropName] = externalPropValue; + mergedProps[propName] = externalPropValue; } return mergedProps; @@ -57,11 +57,12 @@ function merge( function isEventHandler(key: string, value: unknown) { // This approach is more efficient than using a regex. + const thirdCharCode = key.charCodeAt(2); return ( key[0] === 'o' && key[1] === 'n' && - key.charCodeAt(2) >= 65 /* A */ && - key.charCodeAt(2) <= 90 /* Z */ && + thirdCharCode >= 65 /* A */ && + thirdCharCode <= 90 /* Z */ && typeof value === 'function' ); } From 9cefd3229bc26945050633fe752070a613a3e61d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 12 Feb 2025 12:31:29 +0100 Subject: [PATCH 3/5] null check --- packages/react/src/utils/mergeReactProps.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/utils/mergeReactProps.ts b/packages/react/src/utils/mergeReactProps.ts index dd8aa95dbe..2f73b150fd 100644 --- a/packages/react/src/utils/mergeReactProps.ts +++ b/packages/react/src/utils/mergeReactProps.ts @@ -115,6 +115,6 @@ function mergeClassNames(theirClassName: string | undefined, ourClassName: strin return ourClassName; } -function isSyntheticEvent(event: React.SyntheticEvent | {}): event is React.SyntheticEvent { - return typeof event === 'object' && 'nativeEvent' in event; +function isSyntheticEvent(event: React.SyntheticEvent | unknown): event is React.SyntheticEvent { + return event != null && typeof event === 'object' && 'nativeEvent' in event; } From 5c6ed745a9beda7fb268e229bd17ba9c0b08bae2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 12 Feb 2025 13:01:54 +0100 Subject: [PATCH 4/5] nits --- packages/react/src/utils/mergeReactProps.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/utils/mergeReactProps.test.ts b/packages/react/src/utils/mergeReactProps.test.ts index 397c726c2f..c473aa1ed3 100644 --- a/packages/react/src/utils/mergeReactProps.test.ts +++ b/packages/react/src/utils/mergeReactProps.test.ts @@ -16,8 +16,8 @@ describe('mergeReactProps', () => { const mergedProps = mergeReactProps<'button'>(theirProps, ourProps); mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); - mergedProps.onKeyDown?.({ nativeEvent: new MouseEvent('keydown') } as any); - mergedProps.onPaste?.({ nativeEvent: new Event('paste') } as any); + mergedProps.onKeyDown?.({ nativeEvent: new KeyboardEvent('keydown') } as any); + mergedProps.onPaste?.({ nativeEvent: new ClipboardEvent('paste') } as any); expect(theirProps.onClick.calledBefore(ourProps.onClick)).to.equal(true); expect(theirProps.onClick.callCount).to.equal(1); From b898f3387904650d04f09400520b2da0d6972f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Wed, 12 Feb 2025 14:39:56 +0100 Subject: [PATCH 5/5] refactor --- .../react/src/utils/mergeReactProps.test.ts | 24 ++++---- packages/react/src/utils/mergeReactProps.ts | 56 +++++++++++++------ 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/packages/react/src/utils/mergeReactProps.test.ts b/packages/react/src/utils/mergeReactProps.test.ts index c473aa1ed3..1cc55f159e 100644 --- a/packages/react/src/utils/mergeReactProps.test.ts +++ b/packages/react/src/utils/mergeReactProps.test.ts @@ -17,7 +17,7 @@ describe('mergeReactProps', () => { mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); mergedProps.onKeyDown?.({ nativeEvent: new KeyboardEvent('keydown') } as any); - mergedProps.onPaste?.({ nativeEvent: new ClipboardEvent('paste') } as any); + mergedProps.onPaste?.({ nativeEvent: new Event('paste') } as any); expect(theirProps.onClick.calledBefore(ourProps.onClick)).to.equal(true); expect(theirProps.onClick.callCount).to.equal(1); @@ -70,9 +70,8 @@ describe('mergeReactProps', () => { const theirProps = { style: { color: 'red' }, }; - const ourProps = { - style: undefined, - }; + const ourProps = {}; + const mergedProps = mergeReactProps<'button'>(theirProps, ourProps); expect(mergedProps.style).to.deep.equal({ @@ -81,12 +80,8 @@ describe('mergeReactProps', () => { }); it('does not merge styles if both are undefined', () => { - const theirProps = { - style: undefined, - }; - const ourProps = { - style: undefined, - }; + const theirProps = {}; + const ourProps = {}; const mergedProps = mergeReactProps<'button'>(theirProps, ourProps); expect(mergedProps.style).to.equal(undefined); @@ -116,23 +111,24 @@ describe('mergeReactProps', () => { const mergedProps = mergeReactProps<'button'>( { - onClick(event) { + onClick: function onClick1(event) { event.preventBaseUIHandler(); }, }, { - onClick() { + onClick: function onClick2() { ran = true; }, }, { - onClick() { + onClick: function onClick3() { ran = true; }, }, ); - mergedProps.onClick?.({ nativeEvent: new MouseEvent('click') } as any); + const event = { nativeEvent: new MouseEvent('click') } as any; + mergedProps.onClick?.(event); expect(ran).to.equal(false); }); diff --git a/packages/react/src/utils/mergeReactProps.ts b/packages/react/src/utils/mergeReactProps.ts index 2f73b150fd..4633f3a7b0 100644 --- a/packages/react/src/utils/mergeReactProps.ts +++ b/packages/react/src/utils/mergeReactProps.ts @@ -2,35 +2,54 @@ import * as React from 'react'; import type { BaseUIEvent, WithBaseUIEvent } from './types'; /** - * Merges multiple sets of React props such that their event handlers are called in sequence (the user's - * before our internal ones), and allows the user to prevent the internal event handlers from being - * executed by attaching a `preventBaseUIHandler` method. It also merges the `className` and `style` props, whereby - * the classes are concatenated and the user's styles overwrite the internal ones. + * Merges multiple sets of React props such that their event handlers are called in sequence + * (the leftmost one being called first), and allows the user to prevent the subsequent event handlers from being + * executed by attaching a `preventBaseUIHandler` method. + * It also merges the `className` and `style` props, whereby the classes are concatenated + * and the leftmost styles overwrite the subsequent ones. * @important **`ref` is not merged.** - * @param externalProps the user's external props. - * @param internalProps our own internal props. + * @param props props to merge. * @returns the merged props. */ export function mergeReactProps( - externalProps: WithBaseUIEvent> | undefined, - ...internalProps: React.ComponentPropsWithRef[] + ...props: (WithBaseUIEvent> | undefined)[] ): WithBaseUIEvent> { - let mergedInternalProps: WithBaseUIEvent> = internalProps[0]; - for (let i = 1; i < internalProps.length; i += 1) { - mergedInternalProps = merge(mergedInternalProps, internalProps[i]); + if (props.length === 0) { + return {} as WithBaseUIEvent>; + } + + if (props.length === 1) { + return props[0] ?? ({} as WithBaseUIEvent>); } - return merge(externalProps, mergedInternalProps as React.ComponentPropsWithRef); + let merged = merge(props[props.length - 2], props[props.length - 1]); + + for (let i = props.length - 3; i >= 0; i -= 1) { + merged = merge(props[i], merged); + } + + return merged ?? ({} as WithBaseUIEvent>); } +/** + * Merges two sets of props. In case of conflicts, the external props take precedence. + */ function merge( externalProps: WithBaseUIEvent> | undefined, - internalProps: React.ComponentPropsWithRef, + internalProps: WithBaseUIEvent> | undefined, ): WithBaseUIEvent> { if (!externalProps) { + if (!internalProps) { + return {} as WithBaseUIEvent>; + } + return internalProps; } + if (!internalProps) { + return externalProps; + } + return Object.entries(externalProps).reduce( (mergedProps, [propName, externalPropValue]) => { if (isEventHandler(propName, externalPropValue)) { @@ -51,7 +70,7 @@ function merge( return mergedProps; }, - { ...internalProps }, + { ...internalProps } as React.ComponentPropsWithRef, ); } @@ -67,8 +86,8 @@ function isEventHandler(key: string, value: unknown) { ); } -function mergeEventHandlers(theirHandler: any, ourHandler: any) { - return (event: React.SyntheticEvent | {}) => { +function mergeEventHandlers(theirHandler: Function, ourHandler: Function) { + return (event: unknown) => { if (isSyntheticEvent(event)) { let isPrevented = false; const baseUIEvent = event as BaseUIEvent; @@ -97,8 +116,9 @@ function mergeStyles( ourStyle: React.CSSProperties | undefined, ) { if (theirStyle || ourStyle) { - return { ...ourStyle, ...(theirStyle || {}) }; + return { ...ourStyle, ...theirStyle }; } + return undefined; } @@ -115,6 +135,6 @@ function mergeClassNames(theirClassName: string | undefined, ourClassName: strin return ourClassName; } -function isSyntheticEvent(event: React.SyntheticEvent | unknown): event is React.SyntheticEvent { +function isSyntheticEvent(event: unknown): event is React.SyntheticEvent { return event != null && typeof event === 'object' && 'nativeEvent' in event; }