From a11e54d692d3cec4ec2439cbf743b590688fb7d3 Mon Sep 17 00:00:00 2001 From: Mitchell Hamilton Date: Mon, 26 Jul 2021 16:08:53 +1000 Subject: [PATCH] Add react-hooks/exhaustive-deps ESLint rule (#6180) --- .changeset/nine-eagles-develop.md | 10 ++++ .eslintrc.js | 1 + .../packages/tooltip/src/Tooltip.tsx | 6 +- docs/components/MobileMenu.tsx | 2 +- docs/components/Theme.tsx | 2 +- docs/components/docs/DocumentEditorDemo.tsx | 4 +- docs/components/docs/TableOfContents.tsx | 4 +- docs/components/primitives/Emoji.tsx | 1 + packages/auth/src/pages/InitPage.tsx | 4 +- packages/auth/src/pages/SigninPage.tsx | 2 +- packages/cloudinary/src/views/Field.tsx | 1 + .../src/DocumentEditor/Toolbar.tsx | 14 ++--- .../src/DocumentEditor/blockquote.tsx | 2 +- .../src/DocumentEditor/code-block.tsx | 2 +- .../src/DocumentEditor/index.tsx | 5 +- .../src/DocumentEditor/insert-menu.tsx | 2 +- .../src/DocumentEditor/lists.tsx | 2 +- .../fields/src/types/file/views/Field.tsx | 1 + .../fields/src/types/image/views/Field.tsx | 1 + .../admin-ui/pages/ItemPage/index.tsx | 8 ++- .../admin-ui/pages/ListPage/index.tsx | 58 ++++++++++--------- .../src/admin-ui/utils/useAdminMeta.tsx | 6 +- 22 files changed, 79 insertions(+), 59 deletions(-) create mode 100644 .changeset/nine-eagles-develop.md diff --git a/.changeset/nine-eagles-develop.md b/.changeset/nine-eagles-develop.md new file mode 100644 index 00000000000..b94ad151691 --- /dev/null +++ b/.changeset/nine-eagles-develop.md @@ -0,0 +1,10 @@ +--- +'@keystone-ui/tooltip': patch +'@keystone-next/auth': patch +'@keystone-next/cloudinary': patch +'@keystone-next/fields': patch +'@keystone-next/fields-document': patch +'@keystone-next/keystone': patch +--- + +Fixed issues with React hooks dependency arrays diff --git a/.eslintrc.js b/.eslintrc.js index a19e890f3a4..4213df14866 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -33,6 +33,7 @@ module.exports = { 'no-undef': 'error', 'no-unused-expressions': 'error', 'react-hooks/rules-of-hooks': 'error', + 'react-hooks/exhaustive-deps': 'error', '@typescript-eslint/no-unused-vars': [ 'error', { diff --git a/design-system/packages/tooltip/src/Tooltip.tsx b/design-system/packages/tooltip/src/Tooltip.tsx index c4d0c28f19f..1ad77f33731 100644 --- a/design-system/packages/tooltip/src/Tooltip.tsx +++ b/design-system/packages/tooltip/src/Tooltip.tsx @@ -64,8 +64,8 @@ export const Tooltip = ({ }); const tooltipId = useId(); - const showTooltip = useCallback(() => setOpen(true), []); - const hideTooltip = useCallback(() => setOpen(false), []); + const showTooltip = useCallback(() => setOpen(true), [setOpen]); + const hideTooltip = useCallback(() => setOpen(false), [setOpen]); const internalRef = useRef(null); // avoid overriding the consumer's `onClick` handler @@ -77,7 +77,7 @@ export const Tooltip = ({ return () => triggerEl.removeEventListener('click', hideTooltip); } - }, [isOpen]); + }, [isOpen, hideOnClick, hideTooltip]); return ( diff --git a/docs/components/MobileMenu.tsx b/docs/components/MobileMenu.tsx index 5e4bd911e03..fb828622cf8 100644 --- a/docs/components/MobileMenu.tsx +++ b/docs/components/MobileMenu.tsx @@ -30,7 +30,7 @@ export function MobileMenu({ handleClose }: MobileMenuProps) { return () => { document.body.removeEventListener('keydown', handleEsc); }; - }, []); + }, [mobileNavIsOpen, handleClose]); return ( diff --git a/docs/components/Theme.tsx b/docs/components/Theme.tsx index b39190ba537..86c4a8b19ea 100644 --- a/docs/components/Theme.tsx +++ b/docs/components/Theme.tsx @@ -20,7 +20,7 @@ export function Theme() { if (detectedTheme !== 'light') { setTheme(detectedTheme); } - }); + }, []); return ( { const isShiftPressedRef = useKeyDownRef('Shift'); const editor = useMemo( () => createDocumentEditor(documentFeatures, componentBlocks, emptyObj, isShiftPressedRef), - [documentFeatures] + [documentFeatures, isShiftPressedRef] ); // this is why we're creating the editor ourselves and not using the DocumentEditor component @@ -285,7 +285,7 @@ export const DocumentEditorDemo = () => { // we want to force normalize when the document features change so // that no invalid things exist after a user changes something Editor.normalize(editor, { force: true }); - }, [documentFeatures]); + }, [editor, documentFeatures, isShiftPressedRef]); return (
; headings: Heading[]; }) { - let allIds = headings.map(h => h.id); let [visibleIds, setVisibleIds] = useState>([]); let [lastVisibleId, setLastVisbleId] = useState(null); @@ -40,6 +39,7 @@ export function TableOfContents({ useEffect(() => { if (container.current) { + let allIds = headings.map(h => h.id); const observer = new IntersectionObserver(entries => { entries.forEach(entry => { const targetId: string | null = entry.target.getAttribute('id'); @@ -57,7 +57,7 @@ export function TableOfContents({ }); return () => observer.disconnect(); } - }, [container]); + }, [container, headings]); // catch if we're in a long gap between headings and resolve to the last available. let activeId = visibleIds[0] || lastVisibleId; diff --git a/docs/components/primitives/Emoji.tsx b/docs/components/primitives/Emoji.tsx index a7f3a55491c..4e4db0ba548 100644 --- a/docs/components/primitives/Emoji.tsx +++ b/docs/components/primitives/Emoji.tsx @@ -42,6 +42,7 @@ export function Emoji({ symbol, alt, ...props }: EmojiProps) { const posRef = useRef(null); const [showOnTop, setShownTop] = useState(true); + // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { if (posRef.current && posRef.current.offsetTop - window.pageYOffset < 50) { setShownTop(false); diff --git a/packages/auth/src/pages/InitPage.tsx b/packages/auth/src/pages/InitPage.tsx index 72dc4b27a88..57e6dc22c2a 100644 --- a/packages/auth/src/pages/InitPage.tsx +++ b/packages/auth/src/pages/InitPage.tsx @@ -165,7 +165,7 @@ const InitPage = ({ fieldPaths, listKey, enableWelcome }: InitPageProps) => { fields[fieldPath] = adminMeta.lists[listKey].fields[fieldPath]; }); return fields; - }, [fieldPaths]); + }, [fieldPaths, adminMeta.lists, listKey]); const [value, setValue] = useState(() => { let state: Record = {}; @@ -211,7 +211,7 @@ const InitPage = ({ fieldPaths, listKey, enableWelcome }: InitPageProps) => { router.push((router.query.from as string | undefined) || '/'); } } - }, [rawKeystone.authenticatedItem, router.query.from]); + }, [rawKeystone.authenticatedItem, enableWelcome, router]); return mode === 'init' ? ( diff --git a/packages/auth/src/pages/SigninPage.tsx b/packages/auth/src/pages/SigninPage.tsx index 55564eef6a8..57d6e881181 100644 --- a/packages/auth/src/pages/SigninPage.tsx +++ b/packages/auth/src/pages/SigninPage.tsx @@ -61,7 +61,7 @@ export const SigninPage = ({ if (rawKeystone.authenticatedItem.state === 'authenticated') { router.push((router.query.from as string | undefined) || '/'); } - }, [rawKeystone.authenticatedItem, router.query.from]); + }, [rawKeystone.authenticatedItem, router]); return ( Math.random(), [value]); return ( diff --git a/packages/fields-document/src/DocumentEditor/Toolbar.tsx b/packages/fields-document/src/DocumentEditor/Toolbar.tsx index 67917240656..45569c44f18 100644 --- a/packages/fields-document/src/DocumentEditor/Toolbar.tsx +++ b/packages/fields-document/src/DocumentEditor/Toolbar.tsx @@ -51,7 +51,6 @@ export function Toolbar({ documentFeatures: DocumentFeatures; viewState?: { expanded: boolean; toggle: () => void }; }) { - const ExpandIcon = viewState?.expanded ? Minimize2Icon : Maximize2Icon; const relationship = useContext(DocumentFieldRelationshipsContext); const blockComponent = useContext(ComponentBlockContext); const hasBlockItems = Object.entries(relationship).length || Object.keys(blockComponent).length; @@ -119,8 +118,9 @@ export function Toolbar({ {!!hasBlockItems && } - {useMemo( - () => + {useMemo(() => { + const ExpandIcon = viewState?.expanded ? Minimize2Icon : Maximize2Icon; + return ( viewState && ( {attrs => ( @@ -135,9 +135,9 @@ export function Toolbar({ )} - ), - [viewState] - )} + ) + ); + }, [viewState])} ); } @@ -172,7 +172,7 @@ const MarkButton = forwardRef(function {...restProps} /> ); - }, [editor, isDisabled, isSelected, props]); + }, [editor, isDisabled, isSelected, props, ref]); }); const ToolbarContainer = ({ children }: { children: ReactNode }) => { diff --git a/packages/fields-document/src/DocumentEditor/blockquote.tsx b/packages/fields-document/src/DocumentEditor/blockquote.tsx index a4d842ebf39..1f327fc294d 100644 --- a/packages/fields-document/src/DocumentEditor/blockquote.tsx +++ b/packages/fields-document/src/DocumentEditor/blockquote.tsx @@ -116,7 +116,7 @@ const BlockquoteButton = ({ ), - [attrs, isDisabled, isSelected] + [editor, attrs, isDisabled, isSelected] ); }; export const blockquoteButton = ( diff --git a/packages/fields-document/src/DocumentEditor/code-block.tsx b/packages/fields-document/src/DocumentEditor/code-block.tsx index d9c24460c14..32b0a0f9e59 100644 --- a/packages/fields-document/src/DocumentEditor/code-block.tsx +++ b/packages/fields-document/src/DocumentEditor/code-block.tsx @@ -83,7 +83,7 @@ function CodeButton({ attrs }: { attrs: {} }) { ), - [isDisabled, isSelected, attrs] + [isDisabled, isSelected, attrs, editor] ); } diff --git a/packages/fields-document/src/DocumentEditor/index.tsx b/packages/fields-document/src/DocumentEditor/index.tsx index c30775b42a9..1a698698273 100644 --- a/packages/fields-document/src/DocumentEditor/index.tsx +++ b/packages/fields-document/src/DocumentEditor/index.tsx @@ -230,7 +230,7 @@ export function DocumentEditor({ const [expanded, setExpanded] = useState(false); const editor = useMemo( () => createDocumentEditor(documentFeatures, componentBlocks, relationships, isShiftPressedRef), - [documentFeatures, componentBlocks, relationships] + [documentFeatures, componentBlocks, relationships, isShiftPressedRef] ); return ( @@ -309,6 +309,7 @@ export function DocumentEditorProvider({ relationships: Relationships; documentFeatures: DocumentFeatures; }) { + // eslint-disable-next-line react-hooks/exhaustive-deps const identity = useMemo(() => Math.random().toString(36), [editor]); return ( ); - }, [props, ref, isDisabled, isSelected]); + }, [props, ref, isDisabled, isSelected, editor]); }); export function nestList(editor: Editor) { diff --git a/packages/fields/src/types/file/views/Field.tsx b/packages/fields/src/types/file/views/Field.tsx index 7ddee19a24c..8fb8ecdcd90 100644 --- a/packages/fields/src/types/file/views/Field.tsx +++ b/packages/fields/src/types/file/views/Field.tsx @@ -98,6 +98,7 @@ export function Field({ // Generate a random input key when the value changes, to ensure the file input is unmounted and // remounted (this is the only way to reset its value and ensure onChange will fire again if // the user selects the same file again) + // eslint-disable-next-line react-hooks/exhaustive-deps const inputKey = useMemo(() => Math.random(), [value]); return ( diff --git a/packages/fields/src/types/image/views/Field.tsx b/packages/fields/src/types/image/views/Field.tsx index 91621ae3113..26527ee3dd1 100644 --- a/packages/fields/src/types/image/views/Field.tsx +++ b/packages/fields/src/types/image/views/Field.tsx @@ -106,6 +106,7 @@ export function Field({ // Generate a random input key when the value changes, to ensure the file input is unmounted and // remounted (this is the only way to reset its value and ensure onChange will fire again if // the user selects the same file again) + // eslint-disable-next-line react-hooks/exhaustive-deps const inputKey = useMemo(() => Math.random(), [value]); return ( diff --git a/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ItemPage/index.tsx b/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ItemPage/index.tsx index bc0382bca21..3e8ad8caef1 100644 --- a/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ItemPage/index.tsx +++ b/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ItemPage/index.tsx @@ -161,6 +161,8 @@ function ItemForm({ }); }); }); + const labelFieldValue = itemGetter.data?.[list.labelField]; + const itemId = itemGetter.data?.id!; return ( ) : undefined, - [showDelete, list, itemGetter.data?.[list.labelField], itemGetter.data?.id] + [showDelete, list, labelFieldValue, itemId] )} /> diff --git a/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/index.tsx b/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/index.tsx index d4be1ac7cdd..177dff1c384 100644 --- a/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/index.tsx +++ b/packages/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/index.tsx @@ -74,13 +74,14 @@ let listMetaGraphqlQuery: TypedDocumentNode< } `; +const storeableQueries = ['sortBy', 'fields']; + function useQueryParamsFromLocalStorage(listKey: string) { const router = useRouter(); const localStorageKey = `keystone.list.${listKey}.list.page.info`; - const storeableQueries = ['sortBy', 'fields']; const resetToDefaults = () => { - localStorage.removeItem(`keystone.list.${listKey}.list.page.info`); + localStorage.removeItem(localStorageKey); router.replace({ pathname: router.pathname, }); @@ -88,27 +89,31 @@ function useQueryParamsFromLocalStorage(listKey: string) { // GET QUERY FROM CACHE IF CONDITIONS ARE RIGHT // MERGE QUERY PARAMS FROM CACHE WITH QUERY PARAMS FROM ROUTER - useEffect(() => { - let hasSomeQueryParamsWhichAreAboutListPage = Object.keys(router.query).some(x => { - return x.startsWith('!') || storeableQueries.includes(x); - }); - - if (!hasSomeQueryParamsWhichAreAboutListPage && router.isReady) { - const queryParamsFromLocalStorage = localStorage.getItem(localStorageKey); - let parsed; - try { - parsed = JSON.parse(queryParamsFromLocalStorage!); - } catch (err) {} - if (parsed) { - router.replace({ - query: { - ...router.query, - ...parsed, - }, - }); + useEffect( + () => { + let hasSomeQueryParamsWhichAreAboutListPage = Object.keys(router.query).some(x => { + return x.startsWith('!') || storeableQueries.includes(x); + }); + + if (!hasSomeQueryParamsWhichAreAboutListPage && router.isReady) { + const queryParamsFromLocalStorage = localStorage.getItem(localStorageKey); + let parsed; + try { + parsed = JSON.parse(queryParamsFromLocalStorage!); + } catch (err) {} + if (parsed) { + router.replace({ + query: { + ...router.query, + ...parsed, + }, + }); + } } - } - }, [localStorageKey, router.isReady]); + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [localStorageKey, router.isReady] + ); useEffect(() => { let queryParamsToSerialize: Record = {}; Object.keys(router.query).forEach(key => { @@ -117,12 +122,9 @@ function useQueryParamsFromLocalStorage(listKey: string) { } }); if (Object.keys(queryParamsToSerialize).length) { - localStorage.setItem( - `keystone.list.${listKey}.list.page.info`, - JSON.stringify(queryParamsToSerialize) - ); + localStorage.setItem(localStorageKey, JSON.stringify(queryParamsToSerialize)); } else { - localStorage.removeItem(`keystone.list.${listKey}.list.page.info`); + localStorage.removeItem(localStorageKey); } }, [localStorageKey, router]); @@ -440,7 +442,7 @@ function DeleteManyButton({ } } `, - [list, selectedItems] + [list] ) ); const [isOpen, setIsOpen] = useState(false); diff --git a/packages/keystone/src/admin-ui/utils/useAdminMeta.tsx b/packages/keystone/src/admin-ui/utils/useAdminMeta.tsx index 5e5d2591b3f..aa3e4181b18 100644 --- a/packages/keystone/src/admin-ui/utils/useAdminMeta.tsx +++ b/packages/keystone/src/admin-ui/utils/useAdminMeta.tsx @@ -15,7 +15,7 @@ function useMustRenderServerResult() { useEffect(() => { _mustRenderServerResult = false; forceUpdate(1); - }); + }, []); if (typeof window === 'undefined') { return true; @@ -41,7 +41,7 @@ export function useAdminMeta(adminMetaHash: string, fieldViews: FieldViews) { } catch (err) { return; } - }, []); + }, [adminMetaHash]); // it seems like Apollo doesn't skip the first fetch when using skip: true so we're using useLazyQuery instead const [fetchStaticAdminMeta, { data, error, called }] = useLazyQuery(staticAdminMetaQuery, { @@ -133,7 +133,7 @@ export function useAdminMeta(adminMetaHash: string, fieldViews: FieldViews) { ); } return runtimeAdminMeta; - }, [data, error, adminMetaFromLocalStorage]); + }, [data, error, adminMetaFromLocalStorage, fieldViews]); const mustRenderServerResult = useMustRenderServerResult();