From b2210bd418af1a881dca949a928ca3aecd11595d Mon Sep 17 00:00:00 2001 From: "gitstart-app[bot]" <57568882+gitstart-app[bot]@users.noreply.github.com> Date: Wed, 6 Mar 2024 00:24:20 +0100 Subject: [PATCH] TWNTY-2244 - ESLint rule: enforce usage of .getLoadable() + .getValue() to get atoms (#4143) * ESLint rule: enforce usage of .getLoadable() + .getValue() to get atoms Co-authored-by: Matheus * Merge main Co-authored-by: v1b3m Co-authored-by: Matheus * Fix * Refactor according to review Co-authored-by: v1b3m Co-authored-by: Matheus * Fix linter issue Co-authored-by: v1b3m Co-authored-by: Matheus * Fix linter Co-authored-by: v1b3m Co-authored-by: Matheus --------- Co-authored-by: gitstart-twenty Co-authored-by: Matheus Co-authored-by: v1b3m Co-authored-by: Lucas Bordeau --- .vscode/settings.json | 3 +- packages/twenty-front/.eslintrc.cjs | 1 + .../hooks/useGetRelationMetadata.ts | 2 +- ...RecordBoardDeprecatedCardFieldsInternal.ts | 2 +- ...oveRecordBoardDeprecatedCardIdsInternal.ts | 6 +- ...cordBoardDeprecatedCardSelectedInternal.ts | 8 +- .../useUpdateCompanyBoardColumnsInternal.ts | 8 +- .../record-store/hooks/useSetRecordInStore.ts | 2 +- .../hooks/internal/useLeaveTableFocus.ts | 2 +- .../hooks/internal/useSetRecordTableData.ts | 2 +- .../pipeline/hooks/usePipelineSteps.ts | 12 ++- .../hotkey/hooks/usePreviousHotkeyScope.ts | 4 +- .../hotkey/hooks/useScopedHotkeyCallback.ts | 2 +- .../hotkey/hooks/useSetHotkeyScope.ts | 2 +- tools/eslint-rules/index.ts | 7 ++ ...Loadable-and-getValue-to-get-atoms.spec.ts | 53 +++++++++++ ...e-getLoadable-and-getValue-to-get-atoms.ts | 88 +++++++++++++++++++ 17 files changed, 180 insertions(+), 24 deletions(-) create mode 100644 tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.spec.ts create mode 100644 tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.ts diff --git a/.vscode/settings.json b/.vscode/settings.json index c57b880bcd..299062d40a 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -47,5 +47,6 @@ }, "search.exclude": { "**/.yarn": true, - } + }, + "eslint.debug": true } diff --git a/packages/twenty-front/.eslintrc.cjs b/packages/twenty-front/.eslintrc.cjs index 68b808b3cd..2eb970413f 100644 --- a/packages/twenty-front/.eslintrc.cjs +++ b/packages/twenty-front/.eslintrc.cjs @@ -48,6 +48,7 @@ module.exports = { '@nx/workspace-styled-components-prefixed-with-styled': 'error', '@nx/workspace-no-state-useref': 'error', '@nx/workspace-component-props-naming': 'error', + '@nx/workspace-use-getLoadable-and-getValue-to-get-atoms': 'error', 'react/no-unescaped-entities': 'off', 'react/prop-types': 'off', diff --git a/packages/twenty-front/src/modules/object-metadata/hooks/useGetRelationMetadata.ts b/packages/twenty-front/src/modules/object-metadata/hooks/useGetRelationMetadata.ts index 8b8765d4fe..052caea6c5 100644 --- a/packages/twenty-front/src/modules/object-metadata/hooks/useGetRelationMetadata.ts +++ b/packages/twenty-front/src/modules/object-metadata/hooks/useGetRelationMetadata.ts @@ -46,7 +46,7 @@ export const useGetRelationMetadata = () => objectNameType: 'singular', }), ) - .valueOrThrow(); + .getValue(); if (!relationObjectMetadataItem) return null; diff --git a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRecordBoardDeprecatedCardFieldsInternal.ts b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRecordBoardDeprecatedCardFieldsInternal.ts index 6ee2c04f4d..2618e1bb40 100644 --- a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRecordBoardDeprecatedCardFieldsInternal.ts +++ b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRecordBoardDeprecatedCardFieldsInternal.ts @@ -35,7 +35,7 @@ export const useRecordBoardDeprecatedCardFieldsInternal = ( async ( field: Omit, 'size' | 'position'>, ) => { - const existingFields = await snapshot + const existingFields = snapshot .getLoadable(recordBoardCardFieldsScopedState({ scopeId })) .getValue(); diff --git a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRemoveRecordBoardDeprecatedCardIdsInternal.ts b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRemoveRecordBoardDeprecatedCardIdsInternal.ts index ce9a71e24a..afe852ff26 100644 --- a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRemoveRecordBoardDeprecatedCardIdsInternal.ts +++ b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useRemoveRecordBoardDeprecatedCardIdsInternal.ts @@ -11,16 +11,14 @@ export const useRemoveRecordBoardDeprecatedCardIdsInternal = () => { return useRecoilCallback( ({ snapshot, set }) => (cardIdToRemove: string[]) => { - const boardColumns = snapshot - .getLoadable(boardColumnsState) - .valueOrThrow(); + const boardColumns = snapshot.getLoadable(boardColumnsState).getValue(); boardColumns.forEach((boardColumn) => { const columnCardIds = snapshot .getLoadable( recordBoardCardIdsByColumnIdFamilyState(boardColumn.id), ) - .valueOrThrow(); + .getValue(); set( recordBoardCardIdsByColumnIdFamilyState(boardColumn.id), columnCardIds.filter((cardId) => !cardIdToRemove.includes(cardId)), diff --git a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useSetRecordBoardDeprecatedCardSelectedInternal.ts b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useSetRecordBoardDeprecatedCardSelectedInternal.ts index 93cee6d284..cc443d0c58 100644 --- a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useSetRecordBoardDeprecatedCardSelectedInternal.ts +++ b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useSetRecordBoardDeprecatedCardSelectedInternal.ts @@ -19,7 +19,9 @@ export const useSetRecordBoardDeprecatedCardSelectedInternal = (props: any) => { const setCardSelected = useRecoilCallback( ({ set, snapshot }) => (cardId: string, selected: boolean) => { - const activeCardIds = snapshot.getLoadable(activeCardIdsState).contents; + const activeCardIds = snapshot + .getLoadable(activeCardIdsState) + .getValue(); set(isRecordBoardDeprecatedCardSelectedFamilyState(cardId), selected); set(actionBarOpenState, selected || activeCardIds.length > 0); @@ -39,7 +41,9 @@ export const useSetRecordBoardDeprecatedCardSelectedInternal = (props: any) => { const unselectAllActiveCards = useRecoilCallback( ({ set, snapshot }) => () => { - const activeCardIds = snapshot.getLoadable(activeCardIdsState).contents; + const activeCardIds = snapshot + .getLoadable(activeCardIdsState) + .getValue(); activeCardIds.forEach((cardId: string) => { set(isRecordBoardDeprecatedCardSelectedFamilyState(cardId), false); diff --git a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useUpdateCompanyBoardColumnsInternal.ts b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useUpdateCompanyBoardColumnsInternal.ts index 162bb41237..a0e9630a72 100644 --- a/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useUpdateCompanyBoardColumnsInternal.ts +++ b/packages/twenty-front/src/modules/object-record/record-board-deprecated/hooks/internal/useUpdateCompanyBoardColumnsInternal.ts @@ -68,7 +68,7 @@ export const useUpdateCompanyBoardColumnsInternal = () => { for (const [id, companyProgress] of Object.entries(companyBoardIndex)) { const currentCompanyProgress = snapshot .getLoadable(companyProgressesFamilyState(id)) - .valueOrThrow(); + .getValue(); if (!isDeeplyEqual(currentCompanyProgress, companyProgress)) { set(companyProgressesFamilyState(id), companyProgress); @@ -78,11 +78,11 @@ export const useUpdateCompanyBoardColumnsInternal = () => { const currentPipelineSteps = snapshot .getLoadable(currentPipelineStepsState) - .valueOrThrow(); + .getValue(); const currentBoardColumns = snapshot .getLoadable(boardColumnsState) - .valueOrThrow(); + .getValue(); if (!isDeeplyEqual(pipelineSteps, currentPipelineSteps)) { set(currentPipelineStepsState, pipelineSteps); @@ -133,7 +133,7 @@ export const useUpdateCompanyBoardColumnsInternal = () => { .getLoadable( recordBoardCardIdsByColumnIdFamilyState(boardColumn.id), ) - .valueOrThrow(); + .getValue(); if (!isDeeplyEqual(currentBoardCardIds, boardCardIds)) { set( diff --git a/packages/twenty-front/src/modules/object-record/record-store/hooks/useSetRecordInStore.ts b/packages/twenty-front/src/modules/object-record/record-store/hooks/useSetRecordInStore.ts index fb3f09a1c9..832d7ff11a 100644 --- a/packages/twenty-front/src/modules/object-record/record-store/hooks/useSetRecordInStore.ts +++ b/packages/twenty-front/src/modules/object-record/record-store/hooks/useSetRecordInStore.ts @@ -10,7 +10,7 @@ export const useSetRecordInStore = () => { records.forEach((record) => { const currentRecord = snapshot .getLoadable(recordStoreFamilyState(record.id)) - .valueOrThrow(); + .getValue(); if (JSON.stringify(currentRecord) !== JSON.stringify(record)) { set(recordStoreFamilyState(record.id), record); diff --git a/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useLeaveTableFocus.ts b/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useLeaveTableFocus.ts index 5ce6a307f7..ffb3eb2a8d 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useLeaveTableFocus.ts +++ b/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useLeaveTableFocus.ts @@ -26,7 +26,7 @@ export const useLeaveTableFocus = (recordTableId?: string) => { const currentHotkeyScope = snapshot .getLoadable(currentHotkeyScopeState) - .valueOrThrow(); + .getValue(); if (!isSoftFocusActive) { return; diff --git a/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts b/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts index f6fc8e1df8..01fa47fe3b 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts +++ b/packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts @@ -24,7 +24,7 @@ export const useSetRecordTableData = ({ // TODO: refactor with scoped state later const currentEntity = snapshot .getLoadable(recordStoreFamilyState(entity.id)) - .valueOrThrow(); + .getValue(); if (JSON.stringify(currentEntity) !== JSON.stringify(entity)) { set(recordStoreFamilyState(entity.id), entity); diff --git a/packages/twenty-front/src/modules/pipeline/hooks/usePipelineSteps.ts b/packages/twenty-front/src/modules/pipeline/hooks/usePipelineSteps.ts index f68335f693..36544d32c8 100644 --- a/packages/twenty-front/src/modules/pipeline/hooks/usePipelineSteps.ts +++ b/packages/twenty-front/src/modules/pipeline/hooks/usePipelineSteps.ts @@ -19,8 +19,10 @@ export const usePipelineSteps = () => { const handlePipelineStepAdd = useRecoilCallback( ({ snapshot }) => - async (boardColumn: BoardColumnDefinition) => { - const currentPipeline = await snapshot.getPromise(currentPipelineState); + (boardColumn: BoardColumnDefinition) => { + const currentPipeline = snapshot + .getLoadable(currentPipelineState) + .getValue(); if (!currentPipeline?.id) return; return createOnePipelineStep?.({ @@ -35,8 +37,10 @@ export const usePipelineSteps = () => { const handlePipelineStepDelete = useRecoilCallback( ({ snapshot }) => - async (boardColumnId: string) => { - const currentPipeline = await snapshot.getPromise(currentPipelineState); + (boardColumnId: string) => { + const currentPipeline = snapshot + .getLoadable(currentPipelineState) + .getValue(); if (!currentPipeline?.id) return; return deleteOnePipelineStep?.(boardColumnId); diff --git a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/usePreviousHotkeyScope.ts b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/usePreviousHotkeyScope.ts index 36601c63bc..d332b9df0e 100644 --- a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/usePreviousHotkeyScope.ts +++ b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/usePreviousHotkeyScope.ts @@ -14,7 +14,7 @@ export const usePreviousHotkeyScope = () => { () => { const previousHotkeyScope = snapshot .getLoadable(previousHotkeyScopeState) - .valueOrThrow(); + .getValue(); if (!previousHotkeyScope) { return; @@ -35,7 +35,7 @@ export const usePreviousHotkeyScope = () => { (scope: string, customScopes?: CustomHotkeyScopes) => { const currentHotkeyScope = snapshot .getLoadable(currentHotkeyScopeState) - .valueOrThrow(); + .getValue(); setHotkeyScope(scope, customScopes); set(previousHotkeyScopeState, currentHotkeyScope); diff --git a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useScopedHotkeyCallback.ts b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useScopedHotkeyCallback.ts index 74ea22e95b..6771c525c9 100644 --- a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useScopedHotkeyCallback.ts +++ b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useScopedHotkeyCallback.ts @@ -25,7 +25,7 @@ export const useScopedHotkeyCallback = () => }) => { const currentHotkeyScopes = snapshot .getLoadable(internalHotkeysEnabledScopesState) - .valueOrThrow(); + .getValue(); if (!currentHotkeyScopes.includes(scope)) { if (DEBUG_HOTKEY_SCOPE) { diff --git a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useSetHotkeyScope.ts b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useSetHotkeyScope.ts index 26f29170cc..37b9aaf842 100644 --- a/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useSetHotkeyScope.ts +++ b/packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useSetHotkeyScope.ts @@ -27,7 +27,7 @@ export const useSetHotkeyScope = () => async (hotkeyScopeToSet: string, customScopes?: CustomHotkeyScopes) => { const currentHotkeyScope = snapshot .getLoadable(currentHotkeyScopeState) - .valueOrThrow(); + .getValue(); if (currentHotkeyScope.scope === hotkeyScopeToSet) { if (!isNonNullable(customScopes)) { diff --git a/tools/eslint-rules/index.ts b/tools/eslint-rules/index.ts index 6331e01158..78040c741d 100644 --- a/tools/eslint-rules/index.ts +++ b/tools/eslint-rules/index.ts @@ -30,6 +30,11 @@ import { rule as styledComponentsPrefixedWithStyled, RULE_NAME as styledComponentsPrefixedWithStyledName, } from './rules/styled-components-prefixed-with-styled'; +import { + rule as useGetLoadableAndGetValueToGetAtoms, + RULE_NAME as useGetLoadableAndGetValueToGetAtomsName, +} from './rules/use-getLoadable-and-getValue-to-get-atoms'; + /** * Import your custom workspace rules at the top of this file. * @@ -64,6 +69,8 @@ module.exports = { [sortCssPropertiesAlphabeticallyName]: sortCssPropertiesAlphabetically, [styledComponentsPrefixedWithStyledName]: styledComponentsPrefixedWithStyled, + [useGetLoadableAndGetValueToGetAtomsName]: + useGetLoadableAndGetValueToGetAtoms, [maxConstsPerFileName]: maxConstsPerFile, }, }; diff --git a/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.spec.ts b/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.spec.ts new file mode 100644 index 0000000000..a05bb69500 --- /dev/null +++ b/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.spec.ts @@ -0,0 +1,53 @@ +import { TSESLint } from '@typescript-eslint/utils'; + +import { rule, RULE_NAME } from './use-getLoadable-and-getValue-to-get-atoms'; + +const ruleTester = new TSESLint.RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + ecmaFeatures: { + jsx: true, + }, + }, +}); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: 'const atoms = snapshot.getLoadable(someState).getValue();', + }, + { + code: 'const atoms = snapshot.getLoadable(someState(viewId)).getValue();', + }, + ], + invalid: [ + { + code: 'const atoms = await snapshot.getPromise(someState);', + errors: [ + { + messageId: 'invalidWayToGetAtoms', + }, + ], + output: 'const atoms = snapshot.getLoadable(someState).getValue();', + }, + { + code: 'const atoms = await snapshot.getPromise(someState(viewId));', + errors: [ + { + messageId: 'invalidWayToGetAtoms', + }, + ], + output: + 'const atoms = snapshot.getLoadable(someState(viewId)).getValue();', + }, + { + code: 'const atoms = snapshot.getLoadable(someState).anotherMethod();', + errors: [ + { + messageId: 'invalidWayToGetAtoms', + }, + ], + output: 'const atoms = snapshot.getLoadable(someState).getValue();', + }, + ], +}); diff --git a/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.ts b/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.ts new file mode 100644 index 0000000000..c5dbc20f37 --- /dev/null +++ b/tools/eslint-rules/rules/use-getLoadable-and-getValue-to-get-atoms.ts @@ -0,0 +1,88 @@ +import { ESLintUtils } from '@typescript-eslint/utils'; + +// NOTE: The rule will be available in ESLint configs as "@nx/workspace-usage-getLoadable-and-getValue-to-get-atoms" +export const RULE_NAME = 'use-getLoadable-and-getValue-to-get-atoms'; + +export const rule = ESLintUtils.RuleCreator(() => __filename)({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: 'Ensure you are using getLoadable and getValue', + recommended: 'recommended', + }, + fixable: 'code', + schema: [], + messages: { + redundantAwait: 'Redundant await on non-promise', + invalidAccessorOnSnapshot: + "Expected to use method 'getLoadable()' on 'snapshot' but instead found '{{ propertyName }}'", + invalidWayToGetAtoms: + "Expected to use method 'getValue()' with 'getLoadable()' but instead found '{{ propertyName }}'", + }, + }, + defaultOptions: [], + create: (context) => ({ + AwaitExpression: (node) => { + const { argument, range }: any = node; + if ( + (argument.callee?.object?.callee?.object?.name === 'snapshot' && + argument?.callee?.object?.callee?.property?.name === 'getLoadable') || + (argument.callee?.object?.name === 'snapshot' && + argument?.callee?.property?.name === 'getLoadable') + ) { + // remove await + context.report({ + node, + messageId: 'redundantAwait', + data: { + propertyName: argument.callee.property.name, + }, + fix: (fixer) => fixer.removeRange([range[0], range[0] + 5]), + }); + } + }, + MemberExpression: (node) => { + const { object, property }: any = node; + + if ( + object.callee?.type === 'MemberExpression' && + object.callee.object?.name === 'snapshot' && + object.callee.property?.name === 'getLoadable' + ) { + const propertyName = property.name; + + if (propertyName !== 'getValue') { + context.report({ + node: property, + messageId: 'invalidWayToGetAtoms', + data: { + propertyName, + }, + // replace the property with `getValue` + fix: (fixer) => fixer.replaceText(property, 'getValue'), + }); + } + } + }, + CallExpression: (node) => { + const { callee }: any = node; + + if ( + callee.type === 'MemberExpression' && + callee.object?.name === 'snapshot' && + callee.property?.name === 'getPromise' + ) { + context.report({ + node: callee.property, + messageId: 'invalidAccessorOnSnapshot', + data: { + propertyName: callee.property.name, + }, + // Replace `getPromise` with `getLoadable` + fix: (fixer) => fixer.replaceText(callee.property, 'getLoadable'), + }); + } + }, + }), +});