Skip to content

Fix remaining React strict mode issues #4564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 56 commits into from
Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
6fd9bb2
Fix Virtualizer in react strict mode
devongovett May 20, 2023
927f85f
ListBox
devongovett May 20, 2023
5842075
TagGroup
devongovett May 20, 2023
908f913
Fix Virtualizer auto scroll
devongovett May 20, 2023
cc8ac7b
Fix useUpdateEffect
devongovett May 20, 2023
f07556b
Fix VirtualizerItem
devongovett May 20, 2023
f02b087
Fix TableView loading state
devongovett May 20, 2023
bea06a5
Remove old Virtualizer focus behavior
devongovett May 20, 2023
cf617f3
Fix CardView tests
devongovett May 21, 2023
57344bc
Fix DragManager
devongovett May 21, 2023
3768a93
Fix ListView findDOMNode deprecation
devongovett May 21, 2023
654cedb
Make DragManager cancelation behavior consistent
devongovett May 21, 2023
b80d11b
Improve virtualizer onLoadMore some more
devongovett May 21, 2023
bffe05c
Only dispatch initial fetch once in useAsyncList
devongovett May 21, 2023
eec9f87
Fix breadcrumbs
devongovett May 21, 2023
5d2ed38
Make sure we relayout when virtualizer changes size
devongovett May 21, 2023
94f0491
Fix toast
devongovett May 21, 2023
34137a9
Only emit PressResponder warning once
devongovett May 21, 2023
29a4437
skip bad useId merging test
devongovett May 21, 2023
bb3811b
Centralize onLoadMore logic
devongovett May 21, 2023
bc70323
Fix NumberField and ColorField
devongovett May 21, 2023
b609446
Make useMove use useEffectEvent
devongovett May 21, 2023
9ff42e4
useEffectEvent in useFormattedTextField
devongovett May 21, 2023
e68a3a0
Fix calendar
devongovett May 21, 2023
e97e49e
Get rid of refs in render in color area/wheel and sliders
devongovett May 21, 2023
f6e9d71
Move useClipboard to useEffectEvent
devongovett May 21, 2023
38afc92
Move useEvent to useEffectEvent
devongovett May 21, 2023
cdf4461
Move useTabListState auto selection to a useEffect
devongovett May 21, 2023
c158e6f
Fix table resizing ref in render
devongovett May 21, 2023
994b84d
Fix actiongroup
devongovett May 21, 2023
405f8b3
Centralize useDeepMemo hook
devongovett May 21, 2023
5d01282
Refactor usePress to useEffectEvent
devongovett May 21, 2023
285ebca
Convert useInteractOutside to useEffectEvent
devongovett May 21, 2023
87f0b33
Fix dnd refs in render
devongovett May 21, 2023
4d4b940
Fix refs in ActionBar and DialogContainer
devongovett May 21, 2023
aff5e71
lint
devongovett May 21, 2023
3aea9ba
Fix ref in RAC animations
devongovett May 21, 2023
886084e
Fix SideNav
devongovett May 21, 2023
3433338
Fix DialogContainer again
devongovett May 21, 2023
026148a
Fix React 16
devongovett May 21, 2023
bb42ad9
Fix useControlledState with suspense
devongovett May 22, 2023
c48dcfd
Add lint rule
devongovett May 22, 2023
1413f7c
Don't pass through autoFocus to virtualizer div
devongovett May 22, 2023
d2631cd
Move warnings to useEffect
devongovett May 24, 2023
e852e53
Use Object.is in useUpdateEffect
devongovett May 24, 2023
3a3b7a7
Fix onLoadMore
devongovett May 31, 2023
c9ee394
Fix Page Up/Page Down scroll into view
devongovett May 31, 2023
0e372ef
Focus collection when focusedKey becomes null
devongovett May 31, 2023
8b794f6
lint
devongovett May 31, 2023
c4ce017
Merge branch 'main' into react-strict-mode
devongovett May 31, 2023
427bc08
Merge branch 'main' into react-strict-mode
devongovett May 31, 2023
f29e229
Merge branch 'main' of github.com:adobe/react-spectrum into react-str…
devongovett Jun 1, 2023
2f40aa8
Enable strict mode by default in tests
devongovett Jun 1, 2023
1fceb38
Enable strict mode by default in storybook, and hide in production bu…
devongovett Jun 1, 2023
3703359
Fix React canary
devongovett Jun 1, 2023
11b5be3
Don't register storybook addon in production builds
devongovett Jun 1, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion .storybook/custom-addons/strictmode/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
20 changes: 11 additions & 9 deletions .storybook/custom-addons/strictmode/register.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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} />
});
});
});
}
2 changes: 1 addition & 1 deletion .storybook/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ export const parameters = {

export const decorators = [
withScrollingSwitcher,
withStrictModeSwitcher,
...(process.env.NODE_ENV !== 'production' ? [withStrictModeSwitcher] : []),
withProviderSwitcher
];
189 changes: 189 additions & 0 deletions bin/pure-render.js
Original file line number Diff line number Diff line change
@@ -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;
}
9 changes: 4 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
18 changes: 9 additions & 9 deletions packages/@react-aria/actiongroup/src/useActionGroupItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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, {
Expand Down
20 changes: 9 additions & 11 deletions packages/@react-aria/calendar/src/useCalendarBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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);
}

Expand All @@ -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
Expand Down
10 changes: 2 additions & 8 deletions packages/@react-aria/calendar/src/useCalendarCell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
Loading