diff --git a/change/@fluentui-react-tabster-1db057ab-2f5d-4f1f-9ef3-e36a0a1430e9.json b/change/@fluentui-react-tabster-1db057ab-2f5d-4f1f-9ef3-e36a0a1430e9.json new file mode 100644 index 00000000000000..f73bb04804b5aa --- /dev/null +++ b/change/@fluentui-react-tabster-1db057ab-2f5d-4f1f-9ef3-e36a0a1430e9.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "useMergedTabsterAttributes_unstable now supports attributes that change at runtime, including a changing number of attributes", + "packageName": "@fluentui/react-tabster", + "email": "bsunderhus@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.test.ts b/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.test.ts index 963afde1410329..ef701cc495e75d 100644 --- a/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.test.ts +++ b/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.test.ts @@ -39,4 +39,58 @@ describe('useMergedTabsterAttributes', () => { ); expect(result.current).toEqual({ 'data-tabster': '{"a":"1","b":"2","c":"3"}' }); }); + + describe('dynamic attributes at runtime', () => { + it('should recompute when an attribute value changes', () => { + const { result, rerender } = renderHook( + ({ value }: { value: string }) => useMergedTabsterAttributes_unstable({ 'data-tabster': value }), + { initialProps: { value: '{"a":"1"}' } }, + ); + expect(result.current).toEqual({ 'data-tabster': '{"a":"1"}' }); + + rerender({ value: '{"a":"2"}' }); + expect(result.current).toEqual({ 'data-tabster': '{"a":"2"}' }); + }); + + it('should recompute when the number of attributes grows', () => { + const { result, rerender } = renderHook( + ({ attrs }: { attrs: Array<{ 'data-tabster': string } | null> }) => + useMergedTabsterAttributes_unstable(...attrs), + { initialProps: { attrs: [{ 'data-tabster': '{"a":"1"}' }] as Array<{ 'data-tabster': string } | null> } }, + ); + expect(result.current).toEqual({ 'data-tabster': '{"a":"1"}' }); + + rerender({ attrs: [{ 'data-tabster': '{"a":"1"}' }, { 'data-tabster': '{"b":"2"}' }] }); + expect(result.current).toEqual({ 'data-tabster': '{"a":"1","b":"2"}' }); + }); + + it('should recompute when the number of attributes shrinks', () => { + const { result, rerender } = renderHook( + ({ attrs }: { attrs: Array<{ 'data-tabster': string } | null> }) => + useMergedTabsterAttributes_unstable(...attrs), + { + initialProps: { + attrs: [{ 'data-tabster': '{"a":"1"}' }, { 'data-tabster': '{"b":"2"}' }] as Array<{ + 'data-tabster': string; + } | null>, + }, + }, + ); + expect(result.current).toEqual({ 'data-tabster': '{"a":"1","b":"2"}' }); + + rerender({ attrs: [{ 'data-tabster': '{"a":"1"}' }, null] }); + expect(result.current).toEqual({ 'data-tabster': '{"a":"1"}' }); + }); + + it('should return a stable reference when attributes do not change', () => { + const { result, rerender } = renderHook( + ({ value }: { value: string }) => useMergedTabsterAttributes_unstable({ 'data-tabster': value }), + { initialProps: { value: '{"a":"1"}' } }, + ); + const first = result.current; + + rerender({ value: '{"a":"1"}' }); + expect(result.current).toBe(first); + }); + }); }); diff --git a/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.ts b/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.ts index 705d0d10ecf818..2ffd7eb7505ea1 100644 --- a/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.ts +++ b/packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.ts @@ -7,7 +7,9 @@ import { TABSTER_ATTRIBUTE_NAME } from 'tabster'; /** * Merges a collection of tabster attributes. * - * ⚠️The attributes passed as arguments to this hook cannot change at runtime. + * The result is memoized and only recomputed when the contributing attributes + * actually change, so both the values and the number of attributes may change + * at runtime. * @internal * @param attributes - collection of tabster attributes from other react-tabster hooks * @returns single merged tabster attribute @@ -15,36 +17,50 @@ import { TABSTER_ATTRIBUTE_NAME } from 'tabster'; export const useMergedTabsterAttributes_unstable = ( ...attributes: (Partial | null | undefined)[] ): Types.TabsterDOMAttribute => { - const stringAttributes = attributes.reduce((acc, curr) => { - if (curr?.[TABSTER_ATTRIBUTE_NAME]) { - acc.push(curr[TABSTER_ATTRIBUTE_NAME]); + // The collected attributes are reduced to a single primitive `key` so the + // `React.useMemo` dependency list keeps a constant size, even when the number + // of contributing attributes changes between renders. The array and the key + // are built in a single pass. + const stringAttributes: string[] = []; + let key = ''; + for (const attribute of attributes) { + const value = attribute?.[TABSTER_ATTRIBUTE_NAME]; + if (value) { + stringAttributes.push(value); + key += value + MERGE_KEY_SEPARATOR; } - return acc; - }, []); - - if (process.env.NODE_ENV !== 'production') { - // ignoring rules of hooks because this is a condition based on the environment - // it's safe to ignore the rule - // eslint-disable-next-line react-hooks/rules-of-hooks - useWarnIfUnstableAttributes(stringAttributes); } return React.useMemo( - () => ({ - [TABSTER_ATTRIBUTE_NAME]: stringAttributes.length > 0 ? stringAttributes.reduce(mergeJSONStrings) : undefined, - }), - // disable exhaustive-deps because we want to memoize the result of the reduction - // this is safe because the collection of attributes is not expected to change at runtime - // eslint-disable-next-line react-hooks/exhaustive-deps, react-hooks/use-memo - stringAttributes, + () => ({ [TABSTER_ATTRIBUTE_NAME]: mergeAttributes(stringAttributes) }), + // `key` fully captures the contents of `stringAttributes`, which is rebuilt + // on every render and therefore cannot be a dependency itself. + // eslint-disable-next-line react-hooks/exhaustive-deps + [key], ); }; /** - * Merges two JSON strings into one. + * `NUL` separator used to build the memoization key. It is a safe separator + * because the attribute values originate from `JSON.stringify`, which escapes + * control characters, so a literal `NUL` can never appear inside one of them. + */ +const MERGE_KEY_SEPARATOR = '\u0000'; + +/** + * Merges a collection of tabster attribute JSON strings into a single one. + * Later attributes take priority over earlier ones. */ -const mergeJSONStrings = (a: string, b: string): string => - JSON.stringify(Object.assign(safelyParseJSON(a), safelyParseJSON(b))); +const mergeAttributes = (stringAttributes: string[]): string | undefined => { + if (stringAttributes.length === 0) { + return undefined; + } + // common case: a single hook contributed an attribute, no parsing required + if (stringAttributes.length === 1) { + return stringAttributes[0]; + } + return JSON.stringify(Object.assign({}, ...stringAttributes.map(safelyParseJSON))); +}; /** * Tries to parse a JSON string and returns an object. @@ -57,38 +73,3 @@ const safelyParseJSON = (json: string): object => { return {}; } }; - -/** - * Helper hook that ensures that the attributes passed to the hook are stable. - * This is necessary because the attributes are expected to not change at runtime. - * - * This hook will console.warn if the attributes change at runtime. - */ -const useWarnIfUnstableAttributes = (attributes: string[]) => { - const initialAttributesRef = React.useRef(attributes); - - // eslint-disable-next-line react-hooks/refs - let isStable = initialAttributesRef.current.length === attributes.length; - // eslint-disable-next-line react-hooks/refs - if (initialAttributesRef.current !== attributes && isStable) { - for (let i = 0; i < attributes.length; i++) { - // eslint-disable-next-line react-hooks/refs - if (initialAttributesRef.current[i] !== attributes[i]) { - isStable = false; - break; - } - } - } - React.useEffect(() => { - if (!isStable) { - const error = new Error(); - // eslint-disable-next-line no-console - console.warn(/** #__DE-INDENT__ */ ` - @fluentui/react-tabster [useMergedTabsterAttributes]: - The attributes passed to the hook changed at runtime. - This might lead to unexpected behavior, please ensure that the attributes are stable. - ${error.stack} - `); - } - }, [isStable]); -};