From 60598bf2355ad186a9a9447822fec49d8cd17986 Mon Sep 17 00:00:00 2001 From: "gitstart-app[bot]" <57568882+gitstart-app[bot]@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:12:17 +0100 Subject: [PATCH] [ESLint rule] prevent useRecoilCallback without a dependency array (#4411) Co-authored-by: gitstart-twenty Co-authored-by: Matheus Co-authored-by: v1b3m --- packages/twenty-front/.eslintrc.cjs | 1 + .../command-menu/hooks/useCommandMenu.ts | 5 +- .../hooks/useRecordActionBar.tsx | 49 ++++++++++++------- .../components/RecordTableWithWrappers.tsx | 17 +++++-- .../hooks/internal/useSetRowSelectedState.ts | 10 ++-- .../developers/hooks/useGeneratedApiKeys.ts | 1 + .../snack-bar-manager/hooks/useSnackBar.tsx | 16 +++--- .../utilities/scroll/hooks/useListenScroll.ts | 46 ++++++++++------- tools/eslint-rules/index.ts | 6 +++ ...ecoilCallback-has-dependency-array.spec.ts | 29 +++++++++++ .../useRecoilCallback-has-dependency-array.ts | 46 +++++++++++++++++ 11 files changed, 174 insertions(+), 52 deletions(-) create mode 100644 tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.spec.ts create mode 100644 tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.ts diff --git a/packages/twenty-front/.eslintrc.cjs b/packages/twenty-front/.eslintrc.cjs index 1b18512621..f19d1c35aa 100644 --- a/packages/twenty-front/.eslintrc.cjs +++ b/packages/twenty-front/.eslintrc.cjs @@ -51,6 +51,7 @@ module.exports = { '@nx/workspace-component-props-naming': 'error', '@nx/workspace-explicit-boolean-predicates-in-if': 'error', '@nx/workspace-use-getLoadable-and-getValue-to-get-atoms': 'error', + '@nx/workspace-useRecoilCallback-has-dependency-array': 'error', 'react/no-unescaped-entities': 'off', 'react/prop-types': 'off', diff --git a/packages/twenty-front/src/modules/command-menu/hooks/useCommandMenu.ts b/packages/twenty-front/src/modules/command-menu/hooks/useCommandMenu.ts index 74307cc1fa..438e3b11d8 100644 --- a/packages/twenty-front/src/modules/command-menu/hooks/useCommandMenu.ts +++ b/packages/twenty-front/src/modules/command-menu/hooks/useCommandMenu.ts @@ -24,10 +24,10 @@ export const useCommandMenu = () => { goBackToPreviousHotkeyScope, } = usePreviousHotkeyScope(); - const openCommandMenu = () => { + const openCommandMenu = useCallback(() => { setIsCommandMenuOpened(true); setHotkeyScopeAndMemorizePreviousScope(AppHotkeyScope.CommandMenuOpen); - }; + }, [setHotkeyScopeAndMemorizePreviousScope, setIsCommandMenuOpened]); const closeCommandMenu = useRecoilCallback( ({ snapshot }) => @@ -60,6 +60,7 @@ export const useCommandMenu = () => { openCommandMenu(); } }, + [closeCommandMenu, openCommandMenu], ); const addToCommandMenu = useCallback( diff --git a/packages/twenty-front/src/modules/object-record/record-action-bar/hooks/useRecordActionBar.tsx b/packages/twenty-front/src/modules/object-record/record-action-bar/hooks/useRecordActionBar.tsx index b0d801c8da..5b18654a2f 100644 --- a/packages/twenty-front/src/modules/object-record/record-action-bar/hooks/useRecordActionBar.tsx +++ b/packages/twenty-front/src/modules/object-record/record-action-bar/hooks/useRecordActionBar.tsx @@ -45,29 +45,40 @@ export const useRecordActionBar = ({ objectNameSingular: objectMetadataItem.nameSingular, }); - const handleFavoriteButtonClick = useRecoilCallback(({ snapshot }) => () => { - if (selectedRecordIds.length > 1) { - return; - } + const handleFavoriteButtonClick = useRecoilCallback( + ({ snapshot }) => + () => { + if (selectedRecordIds.length > 1) { + return; + } - const selectedRecordId = selectedRecordIds[0]; - const selectedRecord = snapshot - .getLoadable(recordStoreFamilyState(selectedRecordId)) - .getValue(); + const selectedRecordId = selectedRecordIds[0]; + const selectedRecord = snapshot + .getLoadable(recordStoreFamilyState(selectedRecordId)) + .getValue(); - const foundFavorite = favorites?.find( - (favorite) => favorite.recordId === selectedRecordId, - ); + const foundFavorite = favorites?.find( + (favorite) => favorite.recordId === selectedRecordId, + ); - const isFavorite = !!selectedRecordId && !!foundFavorite; + const isFavorite = !!selectedRecordId && !!foundFavorite; - if (isFavorite) { - deleteFavorite(foundFavorite.id); - } else if (isDefined(selectedRecord)) { - createFavorite(selectedRecord, objectMetadataItem.nameSingular); - } - callback?.(); - }); + if (isFavorite) { + deleteFavorite(foundFavorite.id); + } else if (isDefined(selectedRecord)) { + createFavorite(selectedRecord, objectMetadataItem.nameSingular); + } + callback?.(); + }, + [ + callback, + createFavorite, + deleteFavorite, + favorites, + objectMetadataItem.nameSingular, + selectedRecordIds, + ], + ); const handleDeleteClick = useCallback(async () => { callback?.(); diff --git a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableWithWrappers.tsx b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableWithWrappers.tsx index 428111a002..284941f750 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableWithWrappers.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableWithWrappers.tsx @@ -4,9 +4,11 @@ import { useRecoilCallback, useRecoilValue } from 'recoil'; import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem'; import { useDeleteOneRecord } from '@/object-record/hooks/useDeleteOneRecord'; +import { FieldMetadata } from '@/object-record/record-field/types/FieldMetadata'; import { RecordTable } from '@/object-record/record-table/components/RecordTable'; import { EntityDeleteContext } from '@/object-record/record-table/contexts/EntityDeleteHookContext'; import { useRecordTableStates } from '@/object-record/record-table/hooks/internal/useRecordTableStates'; +import { ColumnDefinition } from '@/object-record/record-table/types/ColumnDefinition'; import { IconPlus } from '@/ui/display/icon'; import { Button } from '@/ui/input/button/components/Button'; import AnimatedPlaceholder from '@/ui/layout/animated-placeholder/components/AnimatedPlaceholder'; @@ -92,11 +94,16 @@ export const RecordTableWithWrappers = ({ (columns) => { - persistViewFields( - mapColumnDefinitionsToViewFields(columns), - ); - })} + onColumnsChange={useRecoilCallback( + () => (columns) => { + persistViewFields( + mapColumnDefinitionsToViewFields( + columns as ColumnDefinition[], + ), + ); + }, + [persistViewFields], + )} createRecord={createRecord} /> { const { isRowSelectedFamilyState } = useRecordTableStates(recordTableId); - return useRecoilCallback(({ set }) => (rowId: string, selected: boolean) => { - set(isRowSelectedFamilyState(rowId), selected); - }); + return useRecoilCallback( + ({ set }) => + (rowId: string, selected: boolean) => { + set(isRowSelectedFamilyState(rowId), selected); + }, + [isRowSelectedFamilyState], + ); }; diff --git a/packages/twenty-front/src/modules/settings/developers/hooks/useGeneratedApiKeys.ts b/packages/twenty-front/src/modules/settings/developers/hooks/useGeneratedApiKeys.ts index 888337370a..42dbbe0d46 100644 --- a/packages/twenty-front/src/modules/settings/developers/hooks/useGeneratedApiKeys.ts +++ b/packages/twenty-front/src/modules/settings/developers/hooks/useGeneratedApiKeys.ts @@ -8,5 +8,6 @@ export const useGeneratedApiKeys = () => { (apiKeyId: string, apiKey: string | null) => { set(generatedApiKeyFamilyState(apiKeyId), apiKey); }, + [], ); }; diff --git a/packages/twenty-front/src/modules/ui/feedback/snack-bar-manager/hooks/useSnackBar.tsx b/packages/twenty-front/src/modules/ui/feedback/snack-bar-manager/hooks/useSnackBar.tsx index 0731889e6e..d60a9d6b39 100644 --- a/packages/twenty-front/src/modules/ui/feedback/snack-bar-manager/hooks/useSnackBar.tsx +++ b/packages/twenty-front/src/modules/ui/feedback/snack-bar-manager/hooks/useSnackBar.tsx @@ -14,12 +14,16 @@ export const useSnackBar = () => { SnackBarManagerScopeInternalContext, ); - const handleSnackBarClose = useRecoilCallback(({ set }) => (id: string) => { - set(snackBarInternalScopedState({ scopeId }), (prevState) => ({ - ...prevState, - queue: prevState.queue.filter((snackBar) => snackBar.id !== id), - })); - }); + const handleSnackBarClose = useRecoilCallback( + ({ set }) => + (id: string) => { + set(snackBarInternalScopedState({ scopeId }), (prevState) => ({ + ...prevState, + queue: prevState.queue.filter((snackBar) => snackBar.id !== id), + })); + }, + [scopeId], + ); const setSnackBarQueue = useRecoilCallback( ({ set }) => diff --git a/packages/twenty-front/src/modules/ui/utilities/scroll/hooks/useListenScroll.ts b/packages/twenty-front/src/modules/ui/utilities/scroll/hooks/useListenScroll.ts index 825cb82c5d..969f24e0e3 100644 --- a/packages/twenty-front/src/modules/ui/utilities/scroll/hooks/useListenScroll.ts +++ b/packages/twenty-front/src/modules/ui/utilities/scroll/hooks/useListenScroll.ts @@ -12,28 +12,40 @@ export const useListenScroll = ({ }: { scrollableRef: React.RefObject; }) => { - const hideScrollBarsCallback = useRecoilCallback(({ snapshot }) => () => { - const isScrolling = snapshot.getLoadable(isScrollingState()).getValue(); + const hideScrollBarsCallback = useRecoilCallback( + ({ snapshot }) => + () => { + const isScrolling = snapshot.getLoadable(isScrollingState()).getValue(); - if (!isScrolling) { - scrollableRef.current?.classList.remove('scrolling'); - } - }); + if (!isScrolling) { + scrollableRef.current?.classList.remove('scrolling'); + } + }, + [scrollableRef], + ); - const handleScrollStart = useRecoilCallback(({ set }) => (event: Event) => { - set(isScrollingState(), true); - scrollableRef.current?.classList.add('scrolling'); + const handleScrollStart = useRecoilCallback( + ({ set }) => + (event: Event) => { + set(isScrollingState(), true); + scrollableRef.current?.classList.add('scrolling'); - const target = event.target as HTMLElement; + const target = event.target as HTMLElement; - set(scrollTopState(), target.scrollTop); - set(scrollLeftState(), target.scrollLeft); - }); + set(scrollTopState(), target.scrollTop); + set(scrollLeftState(), target.scrollLeft); + }, + [scrollableRef], + ); - const handleScrollEnd = useRecoilCallback(({ set }) => () => { - set(isScrollingState(), false); - debounce(hideScrollBarsCallback, 1000)(); - }); + const handleScrollEnd = useRecoilCallback( + ({ set }) => + () => { + set(isScrollingState(), false); + debounce(hideScrollBarsCallback, 1000)(); + }, + [hideScrollBarsCallback], + ); useEffect(() => { const refTarget = scrollableRef.current; diff --git a/tools/eslint-rules/index.ts b/tools/eslint-rules/index.ts index 95acb386de..54ac86050b 100644 --- a/tools/eslint-rules/index.ts +++ b/tools/eslint-rules/index.ts @@ -38,6 +38,10 @@ import { rule as useGetLoadableAndGetValueToGetAtoms, RULE_NAME as useGetLoadableAndGetValueToGetAtomsName, } from './rules/use-getLoadable-and-getValue-to-get-atoms'; +import { + rule as useRecoilCallbackHasDependencyArray, + RULE_NAME as useRecoilCallbackHasDependencyArrayName, +} from './rules/useRecoilCallback-has-dependency-array'; /** * Import your custom workspace rules at the top of this file. @@ -77,5 +81,7 @@ module.exports = { [useGetLoadableAndGetValueToGetAtomsName]: useGetLoadableAndGetValueToGetAtoms, [maxConstsPerFileName]: maxConstsPerFile, + [useRecoilCallbackHasDependencyArrayName]: + useRecoilCallbackHasDependencyArray, }, }; diff --git a/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.spec.ts b/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.spec.ts new file mode 100644 index 0000000000..a671cfc69c --- /dev/null +++ b/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.spec.ts @@ -0,0 +1,29 @@ +import { TSESLint } from '@typescript-eslint/utils'; + +import { rule, RULE_NAME } from './useRecoilCallback-has-dependency-array'; + +const ruleTester = new TSESLint.RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), +}); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: 'const someValue = useRecoilCallback(() => () => {}, []);', + }, + { + code: 'const someValue = useRecoilCallback(() => () => {}, [dependency]);', + }, + ], + invalid: [ + { + code: 'const someValue = useRecoilCallback(({}) => () => {});', + errors: [ + { + messageId: 'isNecessaryDependencyArray', + }, + ], + output: 'const someValue = useRecoilCallback(({}) => () => {}, []);', + }, + ], +}); diff --git a/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.ts b/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.ts new file mode 100644 index 0000000000..6585d130d4 --- /dev/null +++ b/tools/eslint-rules/rules/useRecoilCallback-has-dependency-array.ts @@ -0,0 +1,46 @@ +import { ESLintUtils } from '@typescript-eslint/utils'; + +// NOTE: The rule will be available in ESLint configs as "@nx/workspace-useRecoilCallback-has-dependency-array" +export const RULE_NAME = 'useRecoilCallback-has-dependency-array'; + +export const rule = ESLintUtils.RuleCreator(() => __filename)({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: 'Ensure `useRecoilCallback` is used with a dependency array', + recommended: 'recommended', + }, + schema: [], + messages: { + isNecessaryDependencyArray: + 'Is necessary dependency array with useRecoilCallback', + }, + fixable: 'code', + }, + defaultOptions: [], + create: (context) => { + return { + CallExpression: (node) => { + const { callee } = node; + if ( + callee.type === 'Identifier' && + callee.name === 'useRecoilCallback' + ) { + const depsArg = node.arguments; + if (depsArg.length === 1) { + context.report({ + node: callee, + messageId: 'isNecessaryDependencyArray', + data: { + callee, + deps: depsArg[0], + }, + fix: (fixer) => fixer.insertTextAfter(depsArg[0], ', []'), + }); + } + } + }, + }; + }, +});