Fix remaining React strict mode issues (#4564)

Author: devongovett

.eslintrc.js

@@ -169,6 +169,7 @@ module.exports = {
     'rulesdir/sort-imports': [ERROR],
     'rulesdir/imports': [ERROR],
     'rulesdir/useLayoutEffectRule': [ERROR],
+    'rulesdir/pure-render': [ERROR],
 
     // jsx-a11y rules
     'jsx-a11y/accessible-emoji': ERROR,

.storybook/custom-addons/strictmode/index.js

@@ -4,7 +4,7 @@ import React, {StrictMode, useEffect, useState} from 'react';
 
 function StrictModeDecorator(props) {
   let {children} = props;
-  let [isStrict, setStrict] = useState(getQueryParams()?.strict === 'true' || false);
+  let [isStrict, setStrict] = useState(getQueryParams()?.strict !== 'false');
 
   useEffect(() => {
     let channel = addons.getChannel();

.storybook/custom-addons/strictmode/register.js

@@ -4,7 +4,7 @@ import React, {useEffect, useState} from 'react';
 
 const StrictModeToolBar = ({api}) => {
   let channel = addons.getChannel();
-  let [isStrict, setStrict] = useState(getQueryParams()?.strict === 'true' || false);
+  let [isStrict, setStrict] = useState(getQueryParams()?.strict !== 'false');
   let onChange = () => {
     setStrict((old) => {
       channel.emit('strict/updated', !old);
@@ -29,12 +29,14 @@ const StrictModeToolBar = ({api}) => {
   );
 };
 
-addons.register('StrictModeSwitcher', (api) => {
-  addons.add('StrictModeSwitcher', {
-    title: 'Strict mode switcher',
-    type: types.TOOL,
-    //👇 Shows the Toolbar UI element if either the Canvas or Docs tab is active
-    match: ({ viewMode }) => !!(viewMode && viewMode.match(/^(story|docs)$/)),
-    render: () => <StrictModeToolBar api={api} />
+if (process.env.NODE_ENV !== 'production') {
+  addons.register('StrictModeSwitcher', (api) => {
+    addons.add('StrictModeSwitcher', {
+      title: 'Strict mode switcher',
+      type: types.TOOL,
+      //👇 Shows the Toolbar UI element if either the Canvas or Docs tab is active
+      match: ({ viewMode }) => !!(viewMode && viewMode.match(/^(story|docs)$/)),
+      render: () => <StrictModeToolBar api={api} />
+    });
   });
-});
+}

.storybook/preview.js

@@ -26,6 +26,6 @@ export const parameters = {
 
 export const decorators = [
   withScrollingSwitcher,
-  withStrictModeSwitcher,
+  ...(process.env.NODE_ENV !== 'production' ? [withStrictModeSwitcher] : []),
   withProviderSwitcher
 ];

bin/pure-render.js

@@ -0,0 +1,189 @@
+/*
+ * Copyright 2020 Adobe. All rights reserved.
+ * This file is licensed to you under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License. You may obtain a copy
+ * of the License at http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under
+ * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
+ * OF ANY KIND, either express or implied. See the License for the specific language
+ * governing permissions and limitations under the License.
+ */
+
+// Copied from https://github.com/facebook/react/pull/24506
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'enforces component render purity',
+      recommended: true,
+      url: 'https://beta.reactjs.org/learn/keeping-components-pure'
+    }
+  },
+  create(context) {
+    return {
+      MemberExpression(member) {
+        // Look for member expressions that look like refs (i.e. `ref.current`).
+        if (
+          member.object.type !== 'Identifier' ||
+          member.computed ||
+          member.property.type !== 'Identifier' ||
+          member.property.name !== 'current'
+        ) {
+          return;
+        }
+
+        // Find the parent function of this node, as well as any if statement matching against the ref value
+        // (i.e. lazy init pattern shown in React docs).
+        let node = member;
+        let fn;
+        let conditional;
+        while (node) {
+          if (
+            node.type === 'FunctionDeclaration' ||
+            node.type === 'FunctionExpression' ||
+            node.type === 'ArrowFunctionExpression'
+          ) {
+            fn = node;
+            break;
+          }
+
+          if (
+            node.type === 'IfStatement' &&
+            node.test.type === 'BinaryExpression' &&
+            (node.test.operator === '==' || node.test.operator === '===') &&
+            isMemberExpressionEqual(node.test.left, member)
+          ) {
+            conditional = node.test;
+          }
+
+          node = node.parent;
+        }
+
+        if (!fn) {
+          return;
+        }
+
+        // Find the variable definition for the object.
+        const variable = getVariable(context.getScope(), member.object.name);
+        if (!variable) {
+          return;
+        }
+
+        // Find the initialization of the variable and see if it's a call to useRef.
+        const refDefinition = variable.defs.find(def => {
+          const init = def.node.init;
+          if (!init) {
+            return false;
+          }
+
+          return (
+            init.type === 'CallExpression' &&
+            ((init.callee.type === 'Identifier' &&
+              init.callee.name === 'useRef') ||
+              (init.callee.type === 'MemberExpression' &&
+                init.callee.object.type === 'Identifier' &&
+                init.callee.object.name === 'React' &&
+                init.callee.property.type === 'Identifier' &&
+                init.callee.property.name === 'useRef')) &&
+            parentFunction(def.node) === fn
+          );
+        });
+
+        if (refDefinition) {
+          // If within an if statement, check if comparing with the initial value passed to useRef.
+          // This indicates the lazy init pattern, which is allowed.
+          if (conditional) {
+            const init = refDefinition.node.init.arguments[0] || {
+              type: 'Identifier',
+              name: 'undefined'
+            };
+            if (isLiteralEqual(conditional.operator, init, conditional.right)) {
+              return;
+            }
+          }
+
+          // Otherwise, report an error for either writing or reading to this ref based on parent expression.
+          context.report({
+            node: member,
+            message:
+              member.parent.type === 'AssignmentExpression' &&
+              member.parent.left === member
+                ? 'Writing to refs during rendering is not allowed. Move this into a useEffect or useLayoutEffect. See https://beta.reactjs.org/apis/useref'
+                : 'Reading from refs during rendering is not allowed. See https://beta.reactjs.org/apis/useref'
+          });
+        }
+      }
+    };
+  }
+};
+
+function getVariable(scope, name) {
+  while (scope) {
+    const variable = scope.set.get(name);
+    if (variable) {
+      return variable;
+    }
+
+    scope = scope.upper;
+  }
+}
+
+function parentFunction(node) {
+  while (node) {
+    if (
+      node.type === 'FunctionDeclaration' ||
+      node.type === 'FunctionExpression' ||
+      node.type === 'ArrowFunctionExpression'
+    ) {
+      return node;
+    }
+
+    node = node.parent;
+  }
+}
+
+function isMemberExpressionEqual(a, b) {
+  if (a === b) {
+    return true;
+  }
+
+  return (
+    a.type === 'MemberExpression' &&
+    b.type === 'MemberExpression' &&
+    a.object.type === 'Identifier' &&
+    b.object.type === 'Identifier' &&
+    a.object.name === b.object.name &&
+    a.property.type === 'Identifier' &&
+    b.property.type === 'Identifier' &&
+    a.property.name === b.property.name
+  );
+}
+
+function isLiteralEqual(operator, a, b) {
+  let aValue, bValue;
+  if (a.type === 'Identifier' && a.name === 'undefined') {
+    aValue = undefined;
+  } else if (a.type === 'Literal') {
+    aValue = a.value;
+  } else {
+    return;
+  }
+
+  if (b.type === 'Identifier' && b.name === 'undefined') {
+    bValue = undefined;
+  } else if (b.type === 'Literal') {
+    bValue = b.value;
+  } else {
+    return;
+  }
+
+  if (operator === '===') {
+    return aValue === bValue;
+  } else if (operator === '==') {
+    // eslint-disable-next-line
+    return aValue == bValue;
+  }
+
+  return false;
+}

package.json

@@ -20,12 +20,11 @@
     "build:chromatic": "CHROMATIC=1 build-storybook -c .chromatic -o dist/$(git rev-parse HEAD)/chromatic",
     "start:docs": "DOCS_ENV=dev parcel 'packages/@react-{spectrum,aria,stately}/*/docs/*.mdx' 'packages/react-aria-components/docs/*.mdx' 'packages/@internationalized/*/docs/*.mdx' 'packages/dev/docs/pages/**/*.mdx'",
     "build:docs": "DOCS_ENV=staging parcel build 'packages/@react-{spectrum,aria,stately}/*/docs/*.mdx' 'packages/react-aria-components/docs/*.mdx' 'packages/@internationalized/*/docs/*.mdx' 'packages/dev/docs/pages/**/*.mdx'",
-    "test": "yarn jest",
-    "test-strict": "cross-env STRICT_MODE=1 yarn jest",
+    "test": "cross-env STRICT_MODE=1 yarn jest",
+    "test-loose": "yarn jest",
     "build": "make build",
-    "test:ssr": "yarn jest --config jest.ssr.config.js",
-    "ci-test": "yarn jest --maxWorkers=2 && yarn test:ssr --runInBand",
-    "ci-test-16": "yarn jest --maxWorkers=2 && yarn test:ssr --runInBand",
+    "test:ssr": "cross-env STRICT_MODE=1 yarn jest --config jest.ssr.config.js",
+    "ci-test": "cross-env STRICT_MODE=1 yarn jest --maxWorkers=2 && cross-env STRICT_MODE=1 yarn test:ssr --runInBand",
     "lint": "concurrently \"yarn check-types\" \"eslint packages --ext .js,.ts,.tsx\" \"node scripts/lint-packages.js\"",
     "jest": "node scripts/jest.js",
     "copyrights": "babel-node --presets @babel/env ./scripts/addHeaders.js",

packages/@react-aria/actiongroup/src/useActionGroupItem.ts

@@ -11,9 +11,9 @@
  */
 
 import {DOMAttributes, FocusableElement} from '@react-types/shared';
-import {Key, RefObject, useEffect, useRef} from 'react';
+import {Key, RefObject, useEffect} from 'react';
 import {ListState} from '@react-stately/list';
-import {mergeProps} from '@react-aria/utils';
+import {mergeProps, useEffectEvent} from '@react-aria/utils';
 import {PressProps} from '@react-aria/interactions';
 
 export interface AriaActionGroupItemProps {
@@ -43,18 +43,18 @@ export function useActionGroupItem<T>(props: AriaActionGroupItemProps, state: Li
   }
 
   let isFocused = props.key === state.selectionManager.focusedKey;
-  let lastRender = useRef({isFocused, state});
-  lastRender.current = {isFocused, state};
+  let onRemovedWithFocus = useEffectEvent(() => {
+    if (isFocused) {
+      state.selectionManager.setFocusedKey(null);
+    }
+  });
 
   // If the focused item is removed from the DOM, reset the focused key to null.
-  // eslint-disable-next-line arrow-body-style
   useEffect(() => {
     return () => {
-      if (lastRender.current.isFocused) {
-        lastRender.current.state.selectionManager.setFocusedKey(null);
-      }
+      onRemovedWithFocus();
     };
-  }, []);
+  }, [onRemovedWithFocus]);
 
   return {
     buttonProps: mergeProps(buttonProps, {

packages/@react-aria/calendar/src/useCalendarBase.ts

@@ -21,7 +21,7 @@ import {hookData, useSelectedDateDescription, useVisibleRangeDescription} from '
 // @ts-ignore
 import intlMessages from '../intl/*.json';
 import {useLocalizedStringFormatter} from '@react-aria/i18n';
-import {useRef} from 'react';
+import {useState} from 'react';
 
 export interface CalendarAria {
   /** Props for the calendar grouping element. */
@@ -71,17 +71,17 @@ export function useCalendarBase(props: CalendarPropsBase & DOMProps & AriaLabeli
   });
 
   // If the next or previous buttons become disabled while they are focused, move focus to the calendar body.
-  let nextFocused = useRef(false);
+  let [nextFocused, setNextFocused] = useState(false);
   let nextDisabled = props.isDisabled || state.isNextVisibleRangeInvalid();
-  if (nextDisabled && nextFocused.current) {
-    nextFocused.current = false;
+  if (nextDisabled && nextFocused) {
+    setNextFocused(false);
     state.setFocused(true);
   }
 
-  let previousFocused = useRef(false);
+  let [previousFocused, setPreviousFocused] = useState(false);
   let previousDisabled = props.isDisabled || state.isPreviousVisibleRangeInvalid();
-  if (previousDisabled && previousFocused.current) {
-    previousFocused.current = false;
+  if (previousDisabled && previousFocused) {
+    setPreviousFocused(false);
     state.setFocused(true);
   }
 
@@ -100,15 +100,13 @@ export function useCalendarBase(props: CalendarPropsBase & DOMProps & AriaLabeli
       onPress: () => state.focusNextPage(),
       'aria-label': stringFormatter.format('next'),
       isDisabled: nextDisabled,
-      onFocus: () => nextFocused.current = true,
-      onBlur: () => nextFocused.current = false
+      onFocusChange: setNextFocused
     },
     prevButtonProps: {
       onPress: () => state.focusPreviousPage(),
       'aria-label': stringFormatter.format('previous'),
       isDisabled: previousDisabled,
-      onFocus: () => previousFocused.current = true,
-      onBlur: () => previousFocused.current = false
+      onFocusChange: setPreviousFocused
     },
     errorMessageProps: {
       id: errorMessageId

packages/@react-aria/calendar/src/useCalendarCell.ts

@@ -13,7 +13,7 @@
 import {CalendarDate, isEqualDay, isSameDay, isToday} from '@internationalized/date';
 import {CalendarState, RangeCalendarState} from '@react-stately/calendar';
 import {DOMAttributes} from '@react-types/shared';
-import {focusWithoutScrolling, getScrollParent, scrollIntoViewport, useDescription} from '@react-aria/utils';
+import {focusWithoutScrolling, getScrollParent, scrollIntoViewport, useDeepMemo, useDescription} from '@react-aria/utils';
 import {getEraFormat, hookData} from './utils';
 import {getInteractionModality, usePress} from '@react-aria/interactions';
 // @ts-ignore
@@ -102,13 +102,7 @@ export function useCalendarCell(props: AriaCalendarCellProps, state: CalendarSta
 
   // For performance, reuse the same date object as before if the new date prop is the same.
   // This allows subsequent useMemo results to be reused.
-  let lastDate = useRef(null);
-  if (lastDate.current && isEqualDay(date, lastDate.current)) {
-    date = lastDate.current;
-  }
-
-  lastDate.current = date;
-
+  date = useDeepMemo<CalendarDate>(date, isEqualDay);
   let nativeDate = useMemo(() => date.toDate(state.timeZone), [date, state.timeZone]);
 
   // aria-label should be localize Day of week, Month, Day and Year without Time.