Start enabling "rules of hooks" lint rules (microsoft#14097)

czearing · Jul 21, 2020 · 63b41e9 · 63b41e9
1 parent 4b8d8bc
commit 63b41e9
Show file tree

Hide file tree

Showing 36 changed files with 248 additions and 121 deletions.
diff --git a/apps/perf-test/src/app/useTimer.ts b/apps/perf-test/src/app/useTimer.ts
@@ -29,6 +29,7 @@ export const useTimer = () => {
         setVisible(false);
       }, 5000);
     }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [isRunning]);
 
   return {

diff --git a/change/@fluentui-eslint-plugin-2020-07-17-21-53-53-eslint-hooks.json b/change/@fluentui-eslint-plugin-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Enable react-hooks lint rules",
+  "packageName": "@fluentui/eslint-plugin",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-07-18T04:52:18.203Z"
+}
diff --git a/change/@fluentui-react-next-2020-07-17-21-53-53-eslint-hooks.json b/change/@fluentui-react-next-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Disable react-hooks lint rules for now",
+  "packageName": "@fluentui/react-next",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "none",
+  "date": "2020-07-18T04:53:18.328Z"
+}
diff --git a/change/@fluentui-react-stylesheets-2020-07-17-21-53-53-eslint-hooks.json b/change/@fluentui-react-stylesheets-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Fix \"rules of hooks\" lint rule violations",
+  "packageName": "@fluentui/react-stylesheets",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-07-18T04:53:28.129Z"
+}
diff --git a/change/@uifabric-date-time-2020-07-17-21-53-53-eslint-hooks.json b/change/@uifabric-date-time-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Disable react-hooks lint rules for now",
+  "packageName": "@uifabric/date-time",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "none",
+  "date": "2020-07-18T04:52:08.572Z"
+}
diff --git a/change/@uifabric-experiments-2020-07-20-10-53-10-eslint-hooks.json b/change/@uifabric-experiments-2020-07-20-10-53-10-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Disable react-hooks lint rules for now.",
+  "packageName": "@uifabric/experiments",
+  "email": "humbertomakotomorimoto@gmail.com",
+  "dependentChangeType": "none",
+  "date": "2020-07-20T17:53:10.538Z"
+}
diff --git a/change/@uifabric-foundation-2020-07-17-21-53-53-eslint-hooks.json b/change/@uifabric-foundation-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Fix \"rules of hooks\" lint rule violations",
+  "packageName": "@uifabric/foundation",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-07-18T04:52:49.183Z"
+}
diff --git a/change/@uifabric-react-hooks-2020-07-17-21-53-53-eslint-hooks.json b/change/@uifabric-react-hooks-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Disable react-hooks lint rules for now",
+  "packageName": "@uifabric/react-hooks",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "none",
+  "date": "2020-07-18T04:53:13.903Z"
+}
diff --git a/change/@uifabric-tsx-editor-2020-07-17-21-53-53-eslint-hooks.json b/change/@uifabric-tsx-editor-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Fix \"rules of hooks\" lint rule violations",
+  "packageName": "@uifabric/tsx-editor",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-07-18T04:53:43.812Z"
+}
diff --git a/change/@uifabric-utilities-2020-07-17-21-53-53-eslint-hooks.json b/change/@uifabric-utilities-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Disable react-hooks lint rules for now",
+  "packageName": "@uifabric/utilities",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "none",
+  "date": "2020-07-18T04:53:53.969Z"
+}
diff --git a/change/office-ui-fabric-react-2020-07-17-21-53-53-eslint-hooks.json b/change/office-ui-fabric-react-2020-07-17-21-53-53-eslint-hooks.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Fix \"rules of hooks\" lint rule violations",
+  "packageName": "office-ui-fabric-react",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-07-18T04:53:02.288Z"
+}
diff --git a/packages/date-time/.eslintrc.json b/packages/date-time/.eslintrc.json
@@ -1,4 +1,9 @@
 {
   "extends": ["plugin:@fluentui/eslint-plugin/react--legacy"],
-  "root": true
+  "root": true,
+  "rules": {
+    // Disable until issues are dealt with
+    "react-hooks/exhaustive-deps": "off",
+    "react-hooks/rules-of-hooks": "off"
+  }
 }
diff --git a/packages/eslint-plugin/package.json b/packages/eslint-plugin/package.json
@@ -23,7 +23,7 @@
     "eslint-plugin-jest": "^23.13.2",
     "eslint-plugin-jsx-a11y": "^6.2.3",
     "eslint-plugin-react": "^7.20.0",
-    "eslint-plugin-react-hooks": "^4.0.4",
+    "eslint-plugin-react-hooks": "^4.0.8",
     "jju": "^1.4.0"
   },
   "devDependencies": {

diff --git a/packages/eslint-plugin/src/configs/react.js b/packages/eslint-plugin/src/configs/react.js
@@ -108,6 +108,8 @@ const config = {
     ],
     'react/no-string-refs': 'error',
     'react/self-closing-comp': 'error',
+    'react-hooks/exhaustive-deps': 'error',
+    'react-hooks/rules-of-hooks': 'error',
 
     // airbnb or other config overrides (some temporary)
     // TODO: determine which rules we want to enable, and make needed changes (separate PR)
@@ -193,9 +195,6 @@ const config = {
     'react/static-property-placement': 'off',
     'spaced-comment': 'off',
 
-    // Enable ASAP (not done in this PR to make resulting changes reviewable)
-    'react-hooks/exhaustive-deps': 'off',
-    'react-hooks/rules-of-hooks': 'off',
     // airbnb options ban for-of which is unnecessary for TS and modern node (https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/style.js#L334)
     // but this is a very powerful rule we may want to use in other ways
     'no-restricted-syntax': 'off',

diff --git a/packages/experiments/.eslintrc.json b/packages/experiments/.eslintrc.json
@@ -2,6 +2,9 @@
   "extends": ["plugin:@fluentui/eslint-plugin/react--legacy"],
   "root": true,
   "rules": {
-    "@typescript-eslint/no-explicit-any": "off"
+    "@typescript-eslint/no-explicit-any": "off",
+    // Disable until issues are dealt with
+    "react-hooks/exhaustive-deps": "off",
+    "react-hooks/rules-of-hooks": "off"
   }
 }
diff --git a/packages/foundation/src/createComponent.tsx b/packages/foundation/src/createComponent.tsx
@@ -47,7 +47,7 @@ export function createComponent<
   const { factoryOptions = {} } = options;
   const { defaultProp } = factoryOptions;
 
-  const result: React.FunctionComponent<TComponentProps> = (
+  const ResultComponent: React.FunctionComponent<TComponentProps> = (
     componentProps: TComponentProps & IStyleableComponentProps<TViewProps, TTokens, TStyleSet>,
   ) => {
     const settings: ICustomizationProps<TViewProps, TTokens, TStyleSet> = _getCustomizations(
@@ -56,13 +56,13 @@ export function createComponent<
       options.fields,
     );
 
-    const useState = options.state;
+    const stateReducer = options.state;
 
-    if (useState) {
+    if (stateReducer) {
       // Don't assume state will return all props, so spread useState result over component props.
       componentProps = {
         ...componentProps,
-        ...useState(componentProps),
+        ...stateReducer(componentProps),
       };
     }
 
@@ -89,19 +89,19 @@ export function createComponent<
     return view(viewProps);
   };
 
-  result.displayName = options.displayName || view.name;
+  ResultComponent.displayName = options.displayName || view.name;
 
   // If a shorthand prop is defined, create a factory for the component.
   // TODO: This shouldn't be a concern of createComponent.. factoryOptions should just be forwarded.
   //       Need to weigh creating default factories on component creation vs. memoizing them on use in slots.tsx.
   if (defaultProp) {
-    (result as ISlotCreator<TComponentProps, any>).create = createFactory(result, { defaultProp });
+    (ResultComponent as ISlotCreator<TComponentProps, any>).create = createFactory(ResultComponent, { defaultProp });
   }
 
-  assign(result, options.statics);
+  assign(ResultComponent, options.statics);
 
   // Later versions of TypeSript should allow us to merge objects in a type safe way and avoid this cast.
-  return result as React.FunctionComponent<TComponentProps> & TStatics;
+  return ResultComponent as React.FunctionComponent<TComponentProps> & TStatics;
 }
 
 /**

diff --git a/packages/foundation/src/next/composed.tsx b/packages/foundation/src/next/composed.tsx
@@ -152,7 +152,14 @@ export function composed<
   const { factoryOptions = {}, view } = options;
   const { defaultProp } = factoryOptions;
 
-  const result: IFoundationComponent<TComponentProps, TTokens, TStyleSet, TViewProps, TComponentSlots, TStatics> = (
+  const ResultComponent: IFoundationComponent<
+    TComponentProps,
+    TTokens,
+    TStyleSet,
+    TViewProps,
+    TComponentSlots,
+    TStatics
+  > = (
     componentProps: TViewProps &
       IStyleableComponentProps<TViewProps, TTokens, TStyleSet> & { children?: React.ReactNode },
   ) => {
@@ -162,13 +169,13 @@ export function composed<
       options.fields,
     );
 
-    const useState = options.state;
+    const stateReducer = options.state;
 
-    if (useState) {
+    if (stateReducer) {
       // Don't assume state will return all props, so spread useState result over component props.
       componentProps = {
         ...componentProps,
-        ...useState(componentProps),
+        ...stateReducer(componentProps),
       };
     }
 
@@ -263,21 +270,28 @@ export function composed<
     return view ? view(viewProps, Slots) : null;
   };
 
-  result.displayName = options.displayName || (view && view.name);
+  ResultComponent.displayName = options.displayName || (view && view.name);
 
   // If a shorthand prop is defined, create a factory for the component.
   // TODO: This shouldn't be a concern of createComponent.. factoryOptions should just be forwarded.
   //       Need to weigh creating default factories on component creation vs. memoizing them on use in slots.tsx.
   if (defaultProp) {
-    (result as ISlotCreator<TComponentProps, any>).create = createFactory(result, { defaultProp });
+    (ResultComponent as ISlotCreator<TComponentProps, any>).create = createFactory(ResultComponent, { defaultProp });
   }
 
-  result.__options = options;
+  ResultComponent.__options = options;
 
-  assign(result, options.statics);
+  assign(ResultComponent, options.statics);
 
   // Later versions of TypeSript should allow us to merge objects in a type safe way and avoid this cast.
-  return result as IFoundationComponent<TComponentProps, TTokens, TStyleSet, TViewProps, TComponentSlots, TStatics> &
+  return ResultComponent as IFoundationComponent<
+    TComponentProps,
+    TTokens,
+    TStyleSet,
+    TViewProps,
+    TComponentSlots,
+    TStatics
+  > &
     TStatics;
 }
 

diff --git a/packages/lists/.eslintrc.json b/packages/lists/.eslintrc.json
@@ -1,4 +1,9 @@
 {
   "extends": ["plugin:@fluentui/eslint-plugin/react--legacy"],
-  "root": true
+  "root": true,
+  "rules": {
+    // Disable until issues are dealt with
+    "react-hooks/exhaustive-deps": "off",
+    "react-hooks/rules-of-hooks": "off"
+  }
 }
diff --git a/...fabric-react/src/components/CommandBar/examples/CommandBar.CommandBarButtonAs.Example.tsx b/...fabric-react/src/components/CommandBar/examples/CommandBar.CommandBarButtonAs.Example.tsx
@@ -87,7 +87,7 @@ const IndividualCommandBarButtonAsExample: React.FunctionComponent<IIndividualCo
         onClick: () => console.log('Download'),
       },
     ];
-  }, [props.onDismissCoachmark, props.isCoachmarkVisible]);
+  }, [onDismissCoachmark, isCoachmarkVisible]);
 
   return (
     <CommandBar

diff --git a/...ges/office-ui-fabric-react/src/components/CommandBar/examples/CommandBar.Lazy.Example.tsx b/...ges/office-ui-fabric-react/src/components/CommandBar/examples/CommandBar.Lazy.Example.tsx
@@ -62,7 +62,7 @@ export const CommandBarLazyExample: React.FunctionComponent = () => {
         },
       },
     ];
-  }, [menuItems]);
+  }, [menuItems, loadItems, onMenuDismissed]);
 
   return (
     <div>

diff --git a/...ic-react/src/components/ContextualMenu/examples/ContextualMenu.CustomMenuItem.Example.tsx b/...ic-react/src/components/ContextualMenu/examples/ContextualMenu.CustomMenuItem.Example.tsx
@@ -6,16 +6,14 @@ import {
   IContextualMenuItemProps,
 } from 'office-ui-fabric-react/lib/ContextualMenu';
 import { DefaultButton } from 'office-ui-fabric-react/lib/Button';
+import { useConst } from '@uifabric/react-hooks';
 
 export const ContextualMenuWithCustomMenuItemExample: React.FunctionComponent = () => {
-  const menuProps: IContextualMenuProps = React.useMemo(
-    () => ({
-      items: menuItems,
-      shouldFocusOnMount: true,
-      contextualMenuItemAs: (props: IContextualMenuItemProps) => <div>Custom rendered {props.item.text}</div>,
-    }),
-    [menuItems],
-  );
+  const menuProps: IContextualMenuProps = useConst(() => ({
+    items: menuItems,
+    shouldFocusOnMount: true,
+    contextualMenuItemAs: (props: IContextualMenuItemProps) => <div>Custom rendered {props.item.text}</div>,
+  }));
 
   return <DefaultButton text="Click for ContextualMenu" menuProps={menuProps} />;
 };

diff --git a/...ic-react/src/components/ContextualMenu/examples/ContextualMenu.CustomMenuList.Example.tsx b/...ic-react/src/components/ContextualMenu/examples/ContextualMenu.CustomMenuList.Example.tsx
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { useConstCallback } from '@uifabric/react-hooks';
+import { useConstCallback, useConst } from '@uifabric/react-hooks';
 import { DefaultButton } from 'office-ui-fabric-react/lib/Button';
 import { ISearchBoxStyles, SearchBox } from 'office-ui-fabric-react/lib/SearchBox';
 import { Icon } from 'office-ui-fabric-react/lib/Icon';
@@ -52,15 +52,12 @@ export const ContextualMenuWithCustomMenuListExample: React.FunctionComponent =
     },
   );
 
-  const menuProps = React.useMemo(
-    () => ({
-      onRenderMenuList: renderMenuList,
-      title: 'Actions',
-      shouldFocusOnMount: true,
-      items,
-    }),
-    [items],
-  );
+  const menuProps = useConst(() => ({
+    onRenderMenuList: renderMenuList,
+    title: 'Actions',
+    shouldFocusOnMount: true,
+    items,
+  }));
 
   return <DefaultButton text="Click for ContextualMenu" menuProps={menuProps} />;
 };

diff --git a/...abric-react/src/components/ContextualMenu/examples/ContextualMenu.Directional.Example.tsx b/...abric-react/src/components/ContextualMenu/examples/ContextualMenu.Directional.Example.tsx
@@ -69,7 +69,7 @@ export const ContextualMenuDirectionalExample: React.FunctionComponent = () => {
       directionalHintFixed: false,
       items: menuItems,
     }),
-    [isBeakVisible, directionalHint, directionalHintForRTL],
+    [isBeakVisible, directionalHint, directionalHintForRTL, useDirectionalHintForRTL],
   );
 
   return (

diff --git a/packages/office-ui-fabric-react/src/components/Dialog/examples/Dialog.Basic.Example.tsx b/packages/office-ui-fabric-react/src/components/Dialog/examples/Dialog.Basic.Example.tsx
@@ -34,7 +34,7 @@ export const DialogBasicExample: React.FunctionComponent = () => {
       styles: dialogStyles,
       dragOptions: isDraggable ? dragOptions : undefined,
     }),
-    [isDraggable],
+    [isDraggable, labelId, subTextId],
   );
 
   return (

diff --git a/packages/office-ui-fabric-react/src/components/HoverCard/HoverCard.doc.tsx b/packages/office-ui-fabric-react/src/components/HoverCard/HoverCard.doc.tsx
@@ -20,27 +20,27 @@ export const HoverCardPageProps: IDocPageProps = {
     'https://github.com/microsoft/fluentui/tree/master/packages/office-ui-fabric-react/src/components/HoverCard',
   examples: [
     {
-      title: 'Example 1: Expanding HoverCard wrapping an element',
+      title: 'Expanding HoverCard wrapping an element',
       code: HoverCardBasicExampleCode,
       view: <HoverCardBasicExample />,
     },
     {
-      title: 'Example 2: Expanding HoverCard using Target, DirectionalHint and custom HotKey',
+      title: 'Expanding HoverCard using target, DirectionalHint and custom hotkey',
       code: HoverCardTargetExampleCode,
       view: <HoverCardTargetExample />,
     },
     {
-      title: 'Example 3: Plain HoverCard wrapping an element',
+      title: 'Plain HoverCard wrapping an element',
       code: HoverCardPlainCardExampleCode,
       view: <HoverCardPlainCardExample />,
     },
     {
-      title: 'Example 4: Plain HoverCard with instant dismiss from within the card button click',
+      title: 'Plain HoverCard with instant dismiss from within the card button click',
       code: HoverCardInstantDismissExampleCode,
       view: <HoverCardInstantDismissExample />,
     },
     {
-      title: 'Example 5: HoverCard using eventListenerTarget to trigger card open',
+      title: 'HoverCard using eventListenerTarget to trigger card open',
       code: HoverCardEventListenerTargetExampleCode,
       view: <HoverCardEventListenerTargetExample />,
     },