From 46c112edfdcb40681a8997ec4f47b413a08fdd14 Mon Sep 17 00:00:00 2001 From: dan Date: Fri, 5 Apr 2024 15:09:35 +0100 Subject: [PATCH] Enforce that text is wrapped in , remaining cases (#3421) * Toggle.Button -> Toggle.ButtonWithText * Simplify Prompt.Cancel/Action * Move lines down for better diff * Remove ButtonWithText * Simplify types * Enforce Button/ButtonText nesting * Add suggested wrapper in linter error * Check ancestry too * Also check literals * Rm ts-ignore --- .eslintrc.js | 15 +- eslint/__tests__/avoid-unwrapped-text.test.js | 339 +++++++++++++++++- eslint/avoid-unwrapped-text.js | 194 +++++++++- src/components/Button.tsx | 21 +- src/components/Lists.tsx | 6 +- .../moderation/LabelsOnMeDialog.tsx | 4 +- src/screens/Login/ChooseAccountForm.tsx | 4 +- src/screens/Login/LoginForm.tsx | 4 +- src/screens/Onboarding/Layout.tsx | 4 +- src/view/com/auth/server-input/index.tsx | 2 +- src/view/screens/Settings/ExportCarDialog.tsx | 4 +- src/view/screens/Storybook/Buttons.tsx | 16 +- src/view/screens/Storybook/Dialogs.tsx | 14 +- src/view/screens/Storybook/Forms.tsx | 4 +- src/view/screens/Storybook/index.tsx | 29 +- 15 files changed, 587 insertions(+), 73 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index df0c76230a..a999fd24b0 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -23,17 +23,12 @@ module.exports = { 'bsky-internal/avoid-unwrapped-text': [ 'error', { - impliedTextComponents: [ - 'Button', // TODO: Not always safe. - 'H1', - 'H2', - 'H3', - 'H4', - 'H5', - 'H6', - 'P', - ], + impliedTextComponents: ['H1', 'H2', 'H3', 'H4', 'H5', 'H6', 'P'], impliedTextProps: [], + suggestedTextWrappers: { + Button: 'ButtonText', + 'ToggleButton.Button': 'ToggleButton.ButtonText', + }, }, ], 'simple-import-sort/imports': [ diff --git a/eslint/__tests__/avoid-unwrapped-text.test.js b/eslint/__tests__/avoid-unwrapped-text.test.js index 7c667b4a8a..a6762b8fd7 100644 --- a/eslint/__tests__/avoid-unwrapped-text.test.js +++ b/eslint/__tests__/avoid-unwrapped-text.test.js @@ -199,7 +199,7 @@ describe('avoid-unwrapped-text', () => { { code: ` -foo }> @@ -281,6 +281,170 @@ function MyText({ foo }) { } `, }, + + { + code: ` + + {'foo'} + + `, + }, + + { + code: ` + + {foo + 'foo'} + + `, + }, + + { + code: ` + + {'foo'} + + `, + }, + + { + code: ` + + {foo['bar'] && } + + `, + }, + + { + code: ` + + {(foo === 'bar') && } + + `, + }, + + { + code: ` + + {(foo !== 'bar') && } + + `, + }, + + { + code: ` + + {\`foo\`} + + `, + }, + + { + code: ` + + {\`foo\`} + + `, + }, + + { + code: ` + + {_(msg\`foo\`)} + + `, + }, + + { + code: ` + + {_(msg\`foo\`)} + + `, + }, + + { + code: ` + + + + + + `, + }, + + { + code: ` + + stuff('foo')}> + + + + `, + }, + + { + code: ` + + {renderItem('foo')} + + `, + }, + + { + code: ` + + {foo === 'foo' && } + + `, + }, + + { + code: ` + + {foo['foo'] && } + + `, + }, + + { + code: ` + + {check('foo') && } + + `, + }, + + { + code: ` + + {foo.bar && } + + `, + }, + + { + code: ` + + {renderItem('foo')} + + `, + }, + + { + code: ` + + {null} + + `, + }, + + { + code: ` + + {null} + + `, + }, ], invalid: [ @@ -455,6 +619,179 @@ function MyText({ foo }) { `, errors: 1, }, + + { + code: ` + + {'foo'} + + `, + errors: 1, + }, + + { + code: ` + + {foo && 'foo'} + + `, + errors: 1, + }, + + { + code: ` + + {'foo'} + + `, + errors: 1, + }, + + { + code: ` + + {foo && {'foo'}} + + `, + errors: 1, + }, + + { + code: ` + + {10} + + `, + errors: 1, + }, + + { + code: ` + + {10} + + `, + errors: 1, + }, + + { + code: ` + + {foo + 10} + + `, + errors: 1, + }, + + { + code: ` + + {\`foo\`} + + `, + errors: 1, + }, + + { + code: ` + + {\`foo\`} + + `, + errors: 1, + }, + + { + code: ` + + {foo + \`foo\`} + + `, + errors: 1, + }, + + { + code: ` + + {_(msg\`foo\`)} + + `, + errors: 1, + }, + + { + code: ` + + {foo + _(msg\`foo\`)} + + `, + errors: 1, + }, + + { + code: ` + + {_(msg\`foo\`)} + + `, + errors: 1, + }, + + { + code: ` + + {foo + _(msg\`foo\`)} + + `, + errors: 1, + }, + + { + code: ` + + foo + + `, + errors: 1, + }, + + { + code: ` + + foo + + `, + errors: 1, + }, + + { + code: ` + + {foo} + + `, + errors: 1, + }, + + { + code: ` + + {'foo'} + + `, + errors: 1, + }, + + { + code: ` +foo +}> + + + `, + errors: 1, + }, ], } diff --git a/eslint/avoid-unwrapped-text.js b/eslint/avoid-unwrapped-text.js index 79d099f00a..eef31f7951 100644 --- a/eslint/avoid-unwrapped-text.js +++ b/eslint/avoid-unwrapped-text.js @@ -33,6 +33,7 @@ exports.create = function create(context) { const options = context.options[0] || {} const impliedTextProps = options.impliedTextProps ?? [] const impliedTextComponents = options.impliedTextComponents ?? [] + const suggestedTextWrappers = options.suggestedTextWrappers ?? {} const textProps = [...impliedTextProps] const textComponents = ['Text', ...impliedTextComponents] @@ -54,13 +55,13 @@ exports.create = function create(context) { return } if (tagName === 'Trans') { - // Skip over it and check above. + // Exit and rely on the traversal for JSXElement (code below). // TODO: Maybe validate that it's present. - parent = parent.parent - continue + return } - let message = 'Wrap this string in .' - if (tagName !== 'View') { + const suggestedWrapper = suggestedTextWrappers[tagName] + let message = `Wrap this string in <${suggestedWrapper ?? 'Text'}>.` + if (tagName !== 'View' && !suggestedWrapper) { message += ' If <' + tagName + @@ -112,6 +113,189 @@ exports.create = function create(context) { continue } }, + Literal(node) { + if (typeof node.value !== 'string' && typeof node.value !== 'number') { + return + } + let parent = node.parent + while (parent) { + if (parent.type === 'JSXElement') { + const tagName = getTagName(parent) + if (isTextComponent(tagName)) { + // We're good. + return + } + if (tagName === 'Trans') { + // Exit and rely on the traversal for JSXElement (code below). + // TODO: Maybe validate that it's present. + return + } + const suggestedWrapper = suggestedTextWrappers[tagName] + let message = `Wrap this string in <${suggestedWrapper ?? 'Text'}>.` + if (tagName !== 'View' && !suggestedWrapper) { + message += + ' If <' + + tagName + + '> is guaranteed to render , ' + + 'rename it to <' + + tagName + + 'Text> or add it to impliedTextComponents.' + } + context.report({ + node, + message, + }) + return + } + + if (parent.type === 'BinaryExpression' && parent.operator === '+') { + parent = parent.parent + continue + } + + if ( + parent.type === 'JSXExpressionContainer' || + parent.type === 'LogicalExpression' + ) { + parent = parent.parent + continue + } + + // Be conservative for other types. + return + } + }, + TemplateLiteral(node) { + let parent = node.parent + while (parent) { + if (parent.type === 'JSXElement') { + const tagName = getTagName(parent) + if (isTextComponent(tagName)) { + // We're good. + return + } + if (tagName === 'Trans') { + // Exit and rely on the traversal for JSXElement (code below). + // TODO: Maybe validate that it's present. + return + } + const suggestedWrapper = suggestedTextWrappers[tagName] + let message = `Wrap this string in <${suggestedWrapper ?? 'Text'}>.` + if (tagName !== 'View' && !suggestedWrapper) { + message += + ' If <' + + tagName + + '> is guaranteed to render , ' + + 'rename it to <' + + tagName + + 'Text> or add it to impliedTextComponents.' + } + context.report({ + node, + message, + }) + return + } + + if ( + parent.type === 'CallExpression' && + parent.callee.type === 'Identifier' && + parent.callee.name === '_' + ) { + // This is a user-facing string, keep going up. + parent = parent.parent + continue + } + + if (parent.type === 'BinaryExpression' && parent.operator === '+') { + parent = parent.parent + continue + } + + if ( + parent.type === 'JSXExpressionContainer' || + parent.type === 'LogicalExpression' || + parent.type === 'TaggedTemplateExpression' + ) { + parent = parent.parent + continue + } + + // Be conservative for other types. + return + } + }, + JSXElement(node) { + if (getTagName(node) !== 'Trans') { + return + } + let parent = node.parent + while (parent) { + if (parent.type === 'JSXElement') { + const tagName = getTagName(parent) + if (isTextComponent(tagName)) { + // We're good. + return + } + if (tagName === 'Trans') { + // Exit and rely on the traversal for this JSXElement. + // TODO: Should nested even be allowed? + return + } + const suggestedWrapper = suggestedTextWrappers[tagName] + let message = `Wrap this in <${suggestedWrapper ?? 'Text'}>.` + if (tagName !== 'View' && !suggestedWrapper) { + message += + ' If <' + + tagName + + '> is guaranteed to render , ' + + 'rename it to <' + + tagName + + 'Text> or add it to impliedTextComponents.' + } + context.report({ + node, + message, + }) + return + } + + if ( + parent.type === 'JSXAttribute' && + parent.name.type === 'JSXIdentifier' && + parent.parent.type === 'JSXOpeningElement' && + parent.parent.parent.type === 'JSXElement' + ) { + const tagName = getTagName(parent.parent.parent) + const propName = parent.name.name + if ( + textProps.includes(tagName + ' ' + propName) || + propName === 'text' || + propName.endsWith('Text') + ) { + // We're good. + return + } + const message = + 'Wrap this in .' + + ' If `' + + propName + + '` is guaranteed to be wrapped in , ' + + 'rename it to `' + + propName + + 'Text' + + '` or add it to impliedTextProps.' + context.report({ + node, + message, + }) + return + } + + parent = parent.parent + continue + } + }, ReturnStatement(node) { let fnScope = context.getScope() while (fnScope && fnScope.type !== 'function') { diff --git a/src/components/Button.tsx b/src/components/Button.tsx index 12b3fe4cbb..33d777971c 100644 --- a/src/components/Button.tsx +++ b/src/components/Button.tsx @@ -12,7 +12,6 @@ import { ViewStyle, } from 'react-native' import {LinearGradient} from 'expo-linear-gradient' -import {Trans} from '@lingui/macro' import {android, atoms as a, flatten, tokens, useTheme} from '#/alf' import {Props as SVGIconProps} from '#/components/icons/common' @@ -59,6 +58,10 @@ export type ButtonState = { export type ButtonContext = VariantProps & ButtonState +type NonTextElements = + | React.ReactElement + | Iterable + export type ButtonProps = Pick< PressableProps, 'disabled' | 'onPress' | 'testID' @@ -68,11 +71,9 @@ export type ButtonProps = Pick< testID?: string label: string style?: StyleProp - children: - | React.ReactNode - | string - | ((context: ButtonContext) => React.ReactNode | string) + children: NonTextElements | ((context: ButtonContext) => NonTextElements) } + export type ButtonTextProps = TextProps & VariantProps & {disabled?: boolean} const Context = React.createContext({ @@ -404,15 +405,7 @@ export function Button({ )} - {/* @ts-ignore */} - {typeof children === 'string' || children?.type === Trans ? ( - /* @ts-ignore */ - {children} - ) : typeof children === 'function' ? ( - children(context) - ) : ( - children - )} + {typeof children === 'function' ? children(context) : children} ) diff --git a/src/components/Lists.tsx b/src/components/Lists.tsx index 605626fef2..89913b12b0 100644 --- a/src/components/Lists.tsx +++ b/src/components/Lists.tsx @@ -6,7 +6,7 @@ import {useLingui} from '@lingui/react' import {cleanError} from 'lib/strings/errors' import {CenteredView} from 'view/com/util/Views' import {atoms as a, useBreakpoints, useTheme} from '#/alf' -import {Button} from '#/components/Button' +import {Button, ButtonText} from '#/components/Button' import {Error} from '#/components/Error' import {Loader} from '#/components/Loader' import {Text} from '#/components/Typography' @@ -87,7 +87,9 @@ function ListFooterMaybeError({ a.py_sm, ]} onPress={onRetry}> - Retry + + Retry + diff --git a/src/components/moderation/LabelsOnMeDialog.tsx b/src/components/moderation/LabelsOnMeDialog.tsx index 95e3d242b9..5cf86644c0 100644 --- a/src/components/moderation/LabelsOnMeDialog.tsx +++ b/src/components/moderation/LabelsOnMeDialog.tsx @@ -244,7 +244,7 @@ function AppealForm({ size="medium" onPress={onPressBack} label={_(msg`Back`)}> - {_(msg`Back`)} + {_(msg`Back`)} diff --git a/src/screens/Login/ChooseAccountForm.tsx b/src/screens/Login/ChooseAccountForm.tsx index 01eca18760..134411903d 100644 --- a/src/screens/Login/ChooseAccountForm.tsx +++ b/src/screens/Login/ChooseAccountForm.tsx @@ -10,7 +10,7 @@ import {useLoggedOutViewControls} from '#/state/shell/logged-out' import * as Toast from '#/view/com/util/Toast' import {atoms as a} from '#/alf' import {AccountList} from '#/components/AccountList' -import {Button} from '#/components/Button' +import {Button, ButtonText} from '#/components/Button' import * as TextField from '#/components/forms/TextField' import {FormContainer} from './FormContainer' @@ -75,7 +75,7 @@ export const ChooseAccountForm = ({ color="secondary" size="medium" onPress={onPressBack}> - {_(msg`Back`)} + {_(msg`Back`)} diff --git a/src/screens/Login/LoginForm.tsx b/src/screens/Login/LoginForm.tsx index 6b1340b95d..debb39bed6 100644 --- a/src/screens/Login/LoginForm.tsx +++ b/src/screens/Login/LoginForm.tsx @@ -237,7 +237,9 @@ export const LoginForm = ({ color="secondary" size="medium" onPress={onPressRetryConnect}> - {_(msg`Retry`)} + + Retry + ) : !serviceDescription ? ( <> diff --git a/src/screens/Onboarding/Layout.tsx b/src/screens/Onboarding/Layout.tsx index cfaf20ffe1..d48234cca8 100644 --- a/src/screens/Onboarding/Layout.tsx +++ b/src/screens/Onboarding/Layout.tsx @@ -17,7 +17,7 @@ import { useTheme, web, } from '#/alf' -import {Button, ButtonIcon} from '#/components/Button' +import {Button, ButtonIcon, ButtonText} from '#/components/Button' import {ChevronLeft_Stroke2_Corner0_Rounded as ChevronLeft} from '#/components/icons/Chevron' import {createPortalGroup} from '#/components/Portal' import {leading, P, Text} from '#/components/Typography' @@ -73,7 +73,7 @@ export function Layout({children}: React.PropsWithChildren<{}>) { onPress={() => onboardDispatch({type: 'skip'})} // DEV ONLY label="Clear onboarding state"> - Clear + Clear )} diff --git a/src/view/com/auth/server-input/index.tsx b/src/view/com/auth/server-input/index.tsx index 2380fffe2e..0d64650ddb 100644 --- a/src/view/com/auth/server-input/index.tsx +++ b/src/view/com/auth/server-input/index.tsx @@ -167,7 +167,7 @@ export function ServerInputDialog({ size="small" onPress={() => control.close()} label={_(msg`Done`)}> - {_(msg`Done`)} + {_(msg`Done`)} diff --git a/src/view/screens/Settings/ExportCarDialog.tsx b/src/view/screens/Settings/ExportCarDialog.tsx index 3ec37e85e5..e901fb0905 100644 --- a/src/view/screens/Settings/ExportCarDialog.tsx +++ b/src/view/screens/Settings/ExportCarDialog.tsx @@ -92,7 +92,9 @@ export function ExportCarDialog({ size={gtMobile ? 'small' : 'large'} onPress={() => control.close()} label={_(msg`Done`)}> - {_(msg`Done`)} + + Done + diff --git a/src/view/screens/Storybook/Buttons.tsx b/src/view/screens/Storybook/Buttons.tsx index ad2fff3f4a..cae8ec3144 100644 --- a/src/view/screens/Storybook/Buttons.tsx +++ b/src/view/screens/Storybook/Buttons.tsx @@ -4,15 +4,15 @@ import {View} from 'react-native' import {atoms as a} from '#/alf' import { Button, - ButtonVariant, ButtonColor, ButtonIcon, ButtonText, + ButtonVariant, } from '#/components/Button' -import {H1} from '#/components/Typography' import {ArrowTopRight_Stroke2_Corner0_Rounded as ArrowTopRight} from '#/components/icons/ArrowTopRight' import {ChevronLeft_Stroke2_Corner0_Rounded as ChevronLeft} from '#/components/icons/Chevron' import {Globe_Stroke2_Corner0_Rounded as Globe} from '#/components/icons/Globe' +import {H1} from '#/components/Typography' export function Buttons() { return ( @@ -29,7 +29,7 @@ export function Buttons() { color={color as ButtonColor} size="large" label="Click here"> - Button + Button ))} @@ -54,7 +54,7 @@ export function Buttons() { color={name as ButtonColor} size="large" label="Click here"> - Button + Button ), @@ -77,7 +77,7 @@ export function Buttons() { color={name as ButtonColor} size="large" label="Click here"> - Button + Button ), diff --git a/src/view/screens/Storybook/Dialogs.tsx b/src/view/screens/Storybook/Dialogs.tsx index 5c5e480fec..4722784cae 100644 --- a/src/view/screens/Storybook/Dialogs.tsx +++ b/src/view/screens/Storybook/Dialogs.tsx @@ -3,7 +3,7 @@ import {View} from 'react-native' import {useDialogStateControlContext} from '#/state/dialogs' import {atoms as a} from '#/alf' -import {Button} from '#/components/Button' +import {Button, ButtonText} from '#/components/Button' import * as Dialog from '#/components/Dialog' import * as Prompt from '#/components/Prompt' import {H3, P} from '#/components/Typography' @@ -26,7 +26,7 @@ export function Dialogs() { basic.open() }} label="Open basic dialog"> - Open all dialogs + Open all dialogs @@ -102,7 +102,7 @@ export function Dialogs() { size="small" onPress={closeAllDialogs} label="Close all dialogs"> - Close all dialogs + Close all dialogs @@ -116,7 +116,7 @@ export function Dialogs() { }) } label="Open basic dialog"> - Close dialog + Close dialog diff --git a/src/view/screens/Storybook/Forms.tsx b/src/view/screens/Storybook/Forms.tsx index b771ad5e02..1e4efdcc7d 100644 --- a/src/view/screens/Storybook/Forms.tsx +++ b/src/view/screens/Storybook/Forms.tsx @@ -2,7 +2,7 @@ import React from 'react' import {View} from 'react-native' import {atoms as a} from '#/alf' -import {Button} from '#/components/Button' +import {Button, ButtonText} from '#/components/Button' import {DateField, LabelText} from '#/components/forms/DateField' import * as TextField from '#/components/forms/TextField' import * as Toggle from '#/components/forms/Toggle' @@ -191,7 +191,7 @@ export function Forms() { setToggleGroupBValues(['a', 'b']) setToggleGroupCValues(['a']) }}> - Reset all toggles + Reset all toggles diff --git a/src/view/screens/Storybook/index.tsx b/src/view/screens/Storybook/index.tsx index 3a2e2f3696..35a6666016 100644 --- a/src/view/screens/Storybook/index.tsx +++ b/src/view/screens/Storybook/index.tsx @@ -1,22 +1,21 @@ import React from 'react' import {View} from 'react-native' -import {CenteredView, ScrollView} from '#/view/com/util/Views' -import {atoms as a, useTheme, ThemeProvider} from '#/alf' import {useSetThemePrefs} from '#/state/shell' -import {Button} from '#/components/Button' - -import {Theming} from './Theming' -import {Typography} from './Typography' -import {Spacing} from './Spacing' +import {CenteredView, ScrollView} from '#/view/com/util/Views' +import {atoms as a, ThemeProvider, useTheme} from '#/alf' +import {Button, ButtonText} from '#/components/Button' +import {Breakpoints} from './Breakpoints' import {Buttons} from './Buttons' -import {Links} from './Links' -import {Forms} from './Forms' import {Dialogs} from './Dialogs' -import {Breakpoints} from './Breakpoints' -import {Shadows} from './Shadows' +import {Forms} from './Forms' import {Icons} from './Icons' +import {Links} from './Links' import {Menus} from './Menus' +import {Shadows} from './Shadows' +import {Spacing} from './Spacing' +import {Theming} from './Theming' +import {Typography} from './Typography' export function Storybook() { const t = useTheme() @@ -33,7 +32,7 @@ export function Storybook() { size="small" label='Set theme to "system"' onPress={() => setColorMode('system')}> - System + System