diff --git a/.changeset/mosaic-import-guards.md b/.changeset/mosaic-import-guards.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/mosaic-import-guards.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/mosaic/AGENTS.md b/packages/mosaic/AGENTS.md index 16abbc46736..ad729e297b9 100644 --- a/packages/mosaic/AGENTS.md +++ b/packages/mosaic/AGENTS.md @@ -86,7 +86,7 @@ A PR that adds, renames or changes the signature of a shared helper in `src/hook ## Lint -Mosaic runs stricter lint rules than the rest of the repo (the `packages/mosaic` block in `eslint.config.mjs`). +Mosaic runs stricter lint rules than the rest of the repo: the `packages/mosaic` block in the root `eslint.config.mjs`, plus the layer import guards in `packages/mosaic/eslint/config.mjs`, which only load through `packages/mosaic/eslint.config.mjs`. Lint with `pnpm lint` from `packages/mosaic`. Running `eslint` from the repo root skips the import guards. Every disable directive in `src` needs a `-- reason` (`@eslint-community/eslint-comments/require-description`). The reason is not a code comment under the root no-comments rule. Write why the rule does not apply here, not what the code does; it is what a reviewer judges. diff --git a/packages/mosaic/eslint.config.mjs b/packages/mosaic/eslint.config.mjs new file mode 100644 index 00000000000..c6cc71ac863 --- /dev/null +++ b/packages/mosaic/eslint.config.mjs @@ -0,0 +1,8 @@ +import path from 'node:path'; + +import rootConfig from '../../eslint.config.mjs'; +import mosaicImportGuards from './eslint/config.mjs'; + +const root = path.resolve(import.meta.dirname, '../..'); + +export default [...rootConfig.map(config => ({ ...config, basePath: root })), ...mosaicImportGuards]; diff --git a/packages/mosaic/eslint/config.mjs b/packages/mosaic/eslint/config.mjs new file mode 100644 index 00000000000..346e3cd2623 --- /dev/null +++ b/packages/mosaic/eslint/config.mjs @@ -0,0 +1,90 @@ +import { noDynamicMessageCatalogs, noModelReactState } from './rules.mjs'; + +export default [ + { + name: 'packages/mosaic - import guard plugin', + files: ['src/**/*'], + plugins: { + mosaic: { + rules: { + 'no-model-react-state': noModelReactState, + 'no-dynamic-message-catalogs': noDynamicMessageCatalogs, + }, + }, + }, + }, + { + name: 'packages/mosaic - views and controllers', + files: ['src/**/*.{view,controller}.{ts,tsx}'], + rules: { + 'mosaic/no-dynamic-message-catalogs': 'error', + '@typescript-eslint/no-restricted-imports': [ + 'error', + { + paths: [ + { + name: '@clerk/shared/react', + importNames: [ + 'useClerk', + 'useUser', + 'useSession', + 'useSessionList', + 'useOrganization', + 'useOrganizationList', + 'useAuth', + ], + message: + 'Clerk data hooks can only be imported in models. Pass plain data and callbacks to controllers and views.', + allowTypeImports: true, + }, + ], + patterns: [ + { + regex: '[.]messages(?:[.]ts)?$', + message: 'Use useMessages() for localized and overridden copy instead of importing catalog values.', + allowTypeImports: true, + }, + { + regex: '(^|/)use-mosaic-environment(?:[.]ts)?$', + importNames: ['useMosaicEnvironment'], + message: 'Read the Mosaic environment in the model and pass plain data to the controller or view.', + allowTypeImports: true, + }, + { + regex: '(^|/)use-mosaic-router(?:[.]ts)?$', + importNames: ['useMosaicRouter'], + message: 'Read the Mosaic router in the model and pass plain callbacks to the controller or view.', + allowTypeImports: true, + }, + { + regex: '(^|/)use-mosaic-support-email(?:[.]ts)?$', + importNames: ['useMosaicSupportEmail'], + message: 'Read the support email in the model and pass plain data to the controller or view.', + allowTypeImports: true, + }, + ], + }, + ], + }, + }, + { + name: 'packages/mosaic - models', + files: ['src/**/*.model.{ts,tsx}'], + rules: { + 'mosaic/no-model-react-state': 'error', + '@typescript-eslint/no-restricted-imports': [ + 'error', + { + paths: [ + { + name: 'react', + importNames: ['useState', 'useReducer'], + message: 'Put interaction state in the controller and pass its results to the view.', + allowTypeImports: true, + }, + ], + }, + ], + }, + }, +]; diff --git a/packages/mosaic/eslint/import-guards.test.mjs b/packages/mosaic/eslint/import-guards.test.mjs new file mode 100644 index 00000000000..156bab14ef5 --- /dev/null +++ b/packages/mosaic/eslint/import-guards.test.mjs @@ -0,0 +1,227 @@ +import path from 'node:path'; + +import { ESLint } from 'eslint'; +import { describe, expect, test } from 'vitest'; + +const root = path.resolve(import.meta.dirname, '../../..'); +const mosaicRoot = path.resolve(import.meta.dirname, '..'); +const rootEslint = new ESLint({ cwd: root }); +const eslint = new ESLint({ + cwd: mosaicRoot, + overrideConfig: { + languageOptions: { + parserOptions: { + projectService: { + allowDefaultProject: [ + 'packages/mosaic/src/features/sample.view.ts', + 'packages/mosaic/src/features/sample.view.tsx', + 'packages/mosaic/src/features/sample.controller.ts', + 'packages/mosaic/src/features/sample.controller.tsx', + 'packages/mosaic/src/features/sample.model.ts', + 'packages/mosaic/src/features/sample.model.tsx', + 'packages/mosaic/src/features/sample.messages.ts', + ], + }, + }, + }, + }, +}); + +async function lint(file, source) { + const [result] = await eslint.lintText(source, { filePath: path.join(mosaicRoot, 'src/features', file) }); + return result.messages; +} + +async function restrictedImports(file, source) { + const messages = await lint(file, source); + expect(messages.find(message => message.fatal)).toBeUndefined(); + return messages + .filter(message => message.ruleId === '@typescript-eslint/no-restricted-imports') + .map(message => message.message); +} + +describe('Mosaic import boundaries', { timeout: 30_000 }, () => { + test.each(['sample.view.ts', 'sample.view.tsx', 'sample.controller.ts', 'sample.controller.tsx'])( + '%s rejects catalog values and Clerk hooks', + async file => { + const messages = await restrictedImports( + file, + [ + "import { messages } from './sample.messages';", + "import { useUser as user } from '@clerk/shared/react';", + "import { useMosaicRouter as router } from '@/hooks/use-mosaic-router';", + ].join('\n'), + ); + + expect(messages).toHaveLength(3); + expect(messages[0]).toContain('useMessages()'); + expect(messages[1]).toContain('model'); + expect(messages[2]).toContain('model'); + }, + ); + + test('blocks both guards in the same view and accepts type-only catalog imports', async () => { + const messages = await restrictedImports( + 'sample.view.tsx', + [ + "import type { MessageKey } from '@/features/sample.messages.ts';", + "import { type OtherMessageKey } from './sample.messages';", + "import { messages } from '@/features/sample.messages.ts';", + "import { useMosaicEnvironment } from '../../hooks/use-mosaic-environment.ts';", + "import { useMosaicSupportEmail } from '../../hooks/use-mosaic-support-email';", + ].join('\n'), + ); + + expect(messages).toHaveLength(3); + expect(messages[0]).toContain('useMessages()'); + }); + + test.each([ + 'useClerk', + 'useUser', + 'useSession', + 'useSessionList', + 'useOrganization', + 'useOrganizationList', + 'useAuth', + ])('rejects aliased %s from shared React in controllers', async name => { + const messages = await restrictedImports( + 'sample.controller.ts', + `import { ${name} as readClerk } from '@clerk/shared/react';`, + ); + expect(messages).toHaveLength(1); + expect(messages[0]).toContain('model'); + }); + + test.each(['sample.model.ts', 'sample.model.tsx'])( + '%s sends React interaction state to the controller', + async file => { + const messages = await restrictedImports(file, "import { useState as state, useReducer } from 'react';"); + + expect(messages).toHaveLength(2); + expect(messages.every(message => message.includes('controller'))).toBe(true); + }, + ); + + test.each([ + "import React from 'react'; React.useState(0);", + "import R from 'react'; R.useReducer(value => value, 0);", + "import React from 'react'; React['useState'](0);", + "import React from 'react'; const { useState: state } = React; state(0);", + "import React from 'react'; const { ['useReducer']: reducer } = React;", + ])('rejects state hooks through the React default binding: %s', async source => { + const messages = await lint('sample.model.ts', source); + expect(messages.find(message => message.fatal)).toBeUndefined(); + expect(messages.filter(message => message.ruleId === 'mosaic/no-model-react-state')).toEqual([ + expect.objectContaining({ + message: 'Put interaction state in the controller and pass its results to the view.', + messageId: 'controllerState', + }), + ]); + }); + + test.each([ + "import React from 'react'; React.useMemo(() => 0, []);", + "import React from 'react'; export type Node = React.ReactNode;", + "import React from 'react'; export type StateHook = typeof React.useState;", + "import type React from 'react'; export type StateHook = typeof React.useState;", + "import React from 'react'; function read(React) { return React.useState(0); }", + 'const React = { useState: value => value }; React.useState(0);', + "import React from './other'; React.useState(0);", + "import React from 'react'; const useState = 'useMemo'; React[useState](() => 0, []);", + "import React from 'react'; const useState = 'useMemo'; const { [useState]: memo } = React;", + ])('preserves unrelated React usage: %s', async source => { + const messages = await lint('sample.model.ts', source); + expect(messages.find(message => message.fatal)).toBeUndefined(); + expect(messages.filter(message => message.ruleId === 'mosaic/no-model-react-state')).toEqual([]); + }); + + test.each(['sample.view.ts', 'sample.view.tsx', 'sample.controller.ts', 'sample.controller.tsx'])( + '%s rejects dynamic catalog imports', + async file => { + for (const source of ["import('./sample.messages');", 'import(`@/features/sample.messages.ts`);']) { + const messages = await lint(file, source); + expect(messages.find(message => message.fatal)).toBeUndefined(); + expect(messages.filter(message => message.ruleId === 'mosaic/no-dynamic-message-catalogs')).toEqual([ + expect.objectContaining({ + message: 'Use useMessages() for localized and overridden copy instead of importing catalog values.', + messageId: 'useMessages', + }), + ]); + } + }, + ); + + test.each([ + ['sample.view.ts', "import('./sample.controller');"], + ['sample.controller.ts', 'import(`./sample.model`);'], + ['sample.view.ts', "export type Messages = typeof import('./sample.messages');"], + ['sample.model.ts', "import('./sample.messages');"], + ['sample.messages.ts', "import('./other.messages');"], + ['sample.controller.ts', "import React from 'react'; React.useState(0);"], + ])('preserves allowed imports in %s: %s', async (file, source) => { + const messages = await lint(file, source); + expect(messages.find(message => message.fatal)).toBeUndefined(); + expect( + messages.filter(message => + ['mosaic/no-model-react-state', 'mosaic/no-dynamic-message-catalogs'].includes(message.ruleId), + ), + ).toEqual([]); + }); + + test('allows model Clerk hooks, controller state, catalog registration and shared utilities', async () => { + expect(await restrictedImports('sample.model.ts', "import { useUser } from '@clerk/shared/react';")).toEqual([]); + expect(await restrictedImports('sample.model.ts', "import { messages } from './sample.messages';")).toEqual([]); + expect(await restrictedImports('sample.controller.ts', "import { useState } from 'react';")).toEqual([]); + expect( + await restrictedImports('sample.controller.ts', "import type { useUser } from '@clerk/shared/react';"), + ).toEqual([]); + expect(await restrictedImports('sample.messages.ts', "import { messages } from './other.messages';")).toEqual([]); + expect( + await restrictedImports('sample.view.tsx', "import { isClerkAPIResponseError } from '@clerk/shared/error';"), + ).toEqual([]); + expect(await restrictedImports('sample.view.tsx', "import { useController } from './sample.controller';")).toEqual( + [], + ); + expect( + await restrictedImports('sample.view.tsx', "import type { CountryIso } from '@clerk/shared/phone';"), + ).toEqual([]); + expect( + await restrictedImports('sample.view.tsx', "import type { ClerkAPIError } from '@clerk/shared/types';"), + ).toEqual([]); + }); + + test('keeps the repository restriction on the shared package root', async () => { + const messages = await lint( + 'sample.view.tsx', + "import { useUser } from '@clerk/shared';\nimport { css } from '@emotion/react';", + ); + expect(messages.filter(message => message.ruleId === 'no-restricted-imports')).toHaveLength(2); + }); +}); + +describe('Mosaic package configuration', { timeout: 30_000 }, () => { + test.each([ + 'src/features/sample.view.tsx', + 'src/features/sample.controller.ts', + 'src/features/sample.model.ts', + 'src/styles/sample.styles.ts', + 'src/primitives/sample.tsx', + ])('%s inherits repository rules without enabling guards at the root', async file => { + const filePath = path.join(mosaicRoot, file); + const rootConfig = await rootEslint.calculateConfigForFile(filePath); + const mosaicConfig = await eslint.calculateConfigForFile(filePath); + const inheritedRules = { ...mosaicConfig.rules }; + + for (const rule of [ + '@typescript-eslint/no-restricted-imports', + 'mosaic/no-model-react-state', + 'mosaic/no-dynamic-message-catalogs', + ]) { + expect(rootConfig.rules[rule]).toBeUndefined(); + delete inheritedRules[rule]; + } + + expect(inheritedRules).toEqual(rootConfig.rules); + }); +}); diff --git a/packages/mosaic/eslint/rules.mjs b/packages/mosaic/eslint/rules.mjs new file mode 100644 index 00000000000..2b4b12a9987 --- /dev/null +++ b/packages/mosaic/eslint/rules.mjs @@ -0,0 +1,79 @@ +function propertyName(node, computed) { + if (!computed && node.type === 'Identifier') { + return node.name; + } + return node.type === 'Literal' ? node.value : undefined; +} + +export const noModelReactState = { + meta: { + type: 'problem', + schema: [], + messages: { + controllerState: 'Put interaction state in the controller and pass its results to the view.', + }, + }, + create(context) { + const stateHooks = new Set(['useState', 'useReducer']); + + return { + ImportDeclaration(node) { + if (node.source.value !== 'react' || node.importKind === 'type') { + return; + } + for (const variable of context.sourceCode.getDeclaredVariables(node)) { + if (!variable.defs.some(definition => definition.node.type === 'ImportDefaultSpecifier')) { + continue; + } + for (const reference of variable.references) { + const usage = reference.identifier.parent; + if ( + usage.type === 'MemberExpression' && + usage.object === reference.identifier && + stateHooks.has(propertyName(usage.property, usage.computed)) + ) { + context.report({ node: usage, messageId: 'controllerState' }); + } + if ( + usage.type === 'VariableDeclarator' && + usage.init === reference.identifier && + usage.id.type === 'ObjectPattern' + ) { + for (const property of usage.id.properties) { + if (property.type === 'Property' && stateHooks.has(propertyName(property.key, property.computed))) { + context.report({ node: property, messageId: 'controllerState' }); + } + } + } + } + } + }, + }; + }, +}; + +export const noDynamicMessageCatalogs = { + meta: { + type: 'problem', + schema: [], + messages: { + useMessages: 'Use useMessages() for localized and overridden copy instead of importing catalog values.', + }, + }, + create(context) { + return { + ImportExpression(node) { + const source = node.source; + const specifier = + source.type === 'Literal' + ? source.value + : source.type === 'TemplateLiteral' && source.expressions.length === 0 + ? source.quasis[0].value.cooked + : undefined; + if (typeof specifier === 'string' && /[.]messages(?:[.]ts)?$/.test(specifier)) { + context.report({ node, messageId: 'useMessages' }); + } + }, + }; + }, +}; diff --git a/packages/mosaic/src/features/api-keys/api-keys-table.controller.ts b/packages/mosaic/src/features/api-keys/api-keys-table.controller.ts index de537cc7bf8..842547d6a1c 100644 --- a/packages/mosaic/src/features/api-keys/api-keys-table.controller.ts +++ b/packages/mosaic/src/features/api-keys/api-keys-table.controller.ts @@ -3,13 +3,15 @@ import { useEffect, useRef, useState } from 'react'; import { useNow } from '../../hooks/use-now'; import { useLocale } from '../../localization'; import { formatDate, formatRelativeTime } from './api-keys-table.format'; -import type { APIKeysTableModel } from './api-keys-table.model'; +import { useAPIKeysTableModel } from './api-keys-table.model'; import type { APIKey, APIKeyRecord, APIKeysTableMessages } from './api-keys-table.types'; import { useCreateAPIKeyController } from './create-api-key.controller'; const SEARCH_DEBOUNCE_MS = 500; -export function useAPIKeysTableController(model: APIKeysTableModel, messages: APIKeysTableMessages) { +export function useAPIKeysTableController(subject: string, messages: APIKeysTableMessages) { + const [query, setQuery] = useState(''); + const model = useAPIKeysTableModel(subject, query); const locale = useLocale(); const now = useNow({ updateInterval: 60_000 }); const [searchValue, setSearchValue] = useState(''); @@ -34,7 +36,8 @@ export function useAPIKeysTableController(model: APIKeysTableModel, messages: AP setSearchValue(value); clearTimeout(timer.current); timer.current = setTimeout(() => { - model.search(value.trim()); + setQuery(value.trim()); + model.fetchPage(1); }, SEARCH_DEBOUNCE_MS); }, manage: model.canManage diff --git a/packages/mosaic/src/features/api-keys/api-keys-table.model.ts b/packages/mosaic/src/features/api-keys/api-keys-table.model.ts index 98741477c56..eb4dd0593f3 100644 --- a/packages/mosaic/src/features/api-keys/api-keys-table.model.ts +++ b/packages/mosaic/src/features/api-keys/api-keys-table.model.ts @@ -1,7 +1,6 @@ import { isOrganizationId } from '@clerk/shared/internal/clerk-js/organization'; import { useAPIKeys, useClerk, useSession } from '@clerk/shared/react'; import type { APIKeyResource } from '@clerk/shared/types'; -import { useState } from 'react'; import { useMosaicEnvironment } from '../../hooks/use-mosaic-environment'; import { save } from '../../utils/errors'; @@ -30,9 +29,8 @@ export function useAPIKeysAccess(subject: string | undefined) { return { isLoaded, isAvailable, canRead, canManage }; } -export function useAPIKeysTableModel(subject: string) { +export function useAPIKeysTableModel(subject: string, query: string) { const clerk = useClerk(); - const [query, setQuery] = useState(''); const { isLoaded, isAvailable, canRead, canManage } = useAPIKeysAccess(subject); const apiKeys = useAPIKeys({ subject, @@ -48,10 +46,6 @@ export function useAPIKeysTableModel(subject: string) { ...apiKeys, data: apiKeys.data.map(toAPIKeyRecord), canManage, - search: (nextQuery: string) => { - setQuery(nextQuery); - apiKeys.fetchPage(1); - }, create: async ({ name, expiresAt }: CreateAPIKeyInput) => { let secret = ''; await save(async () => { diff --git a/packages/mosaic/src/features/api-keys/api-keys-table.tsx b/packages/mosaic/src/features/api-keys/api-keys-table.tsx index f7fbff30ab3..80222108d6a 100644 --- a/packages/mosaic/src/features/api-keys/api-keys-table.tsx +++ b/packages/mosaic/src/features/api-keys/api-keys-table.tsx @@ -4,7 +4,7 @@ import type { ReactNode } from 'react'; import { useMessages } from '../../localization'; import { useAPIKeysTableController } from './api-keys-table.controller'; import { resolveAPIKeysTableMessages } from './api-keys-table.messages'; -import { PAGE_SIZE, useAPIKeysTableModel } from './api-keys-table.model'; +import { PAGE_SIZE } from './api-keys-table.model'; import type { APIKeysTableMessages } from './api-keys-table.types'; import { APIKeysTableView } from './api-keys-table.view'; @@ -29,8 +29,7 @@ function SubjectAPIKeysTable({ subject, messages: overrides, fallback }: APIKeys isOrganizationId(subject) ? 'organization' : 'user', overrides, ); - const model = useAPIKeysTableModel(subject); - const { isLoaded, isAvailable, manage, ...controller } = useAPIKeysTableController(model, messages); + const { isLoaded, isAvailable, manage, ...controller } = useAPIKeysTableController(subject, messages); if (!isLoaded || !isAvailable) { return fallback ?? null;