[pull] master from microsoft:master

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"
+}

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);
+    });
+  });
 });

packages/react-components/react-tabster/src/hooks/useMergeTabsterAttributes.ts

@@ -7,44 +7,60 @@ 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
  */
 export const useMergedTabsterAttributes_unstable = (
   ...attributes: (Partial<Types.TabsterDOMAttribute> | null | undefined)[]
 ): Types.TabsterDOMAttribute => {
-  const stringAttributes = attributes.reduce<string[]>((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]);
-};