From 4cc57f4bfdf16fd627ad63bf20ff8193e7d0a12a Mon Sep 17 00:00:00 2001 From: dan Date: Thu, 4 Apr 2024 17:32:50 +0100 Subject: [PATCH] Lint against strings without wrapping (#3398) * Add a rudimentary rule * Get the rule passing * Support special-casing text props * More tests --- .eslintrc.js | 33 ++ eslint/__tests__/avoid-unwrapped-text.test.js | 423 ++++++++++++++++++ eslint/avoid-unwrapped-text.js | 111 +++++ eslint/index.js | 7 + package.json | 1 + yarn.lock | 4 + 6 files changed, 579 insertions(+) create mode 100644 eslint/__tests__/avoid-unwrapped-text.test.js create mode 100644 eslint/avoid-unwrapped-text.js create mode 100644 eslint/index.js diff --git a/.eslintrc.js b/.eslintrc.js index 6e8e01fe..8ae91f34 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -1,3 +1,5 @@ +const bskyEslint = require('./eslint') + module.exports = { root: true, extends: [ @@ -13,12 +15,43 @@ module.exports = { 'react', 'lingui', 'simple-import-sort', + 'bsky-internal', ], rules: { // Temporary until https://github.com/facebook/react-native/pull/43756 gets into a release. 'prettier/prettier': 0, 'react/no-unescaped-entities': 0, 'react-native/no-inline-styles': 0, + 'bsky-internal/avoid-unwrapped-text': [ + 'error', + { + impliedTextComponents: [ + 'Button', // TODO: Not always safe. + 'ButtonText', + 'DateField.Label', + 'Description', + 'H1', + 'H2', + 'H3', + 'H4', + 'H5', + 'H6', + 'InlineLink', + 'Label', + 'P', + 'Prompt.Title', + 'Prompt.Description', + 'Prompt.Cancel', // TODO: Not always safe. + 'Prompt.Action', // TODO: Not always safe. + 'TextField.Label', + 'TextField.Suffix', + 'Title', + 'Toggle.Label', + 'ToggleButton.Button', // TODO: Not always safe. + ], + impliedTextProps: ['FormContainer title'], + }, + ], 'simple-import-sort/imports': [ 'warn', { diff --git a/eslint/__tests__/avoid-unwrapped-text.test.js b/eslint/__tests__/avoid-unwrapped-text.test.js new file mode 100644 index 00000000..0fbc0112 --- /dev/null +++ b/eslint/__tests__/avoid-unwrapped-text.test.js @@ -0,0 +1,423 @@ +const {RuleTester} = require('eslint') +const avoidUnwrappedText = require('../avoid-unwrapped-text') + +const ruleTester = new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + ecmaFeatures: { + jsx: true, + }, + ecmaVersion: 6, + sourceType: 'module', + }, +}) + +describe('avoid-unwrapped-text', () => { + const tests = { + valid: [ + { + code: ` + + foo + + `, + }, + + { + code: ` + + + foo + + + `, + }, + + { + code: ` + + <> + foo + + + `, + }, + + { + code: ` + + {foo && foo} + + `, + }, + + { + code: ` + + {foo ? foo : bar} + + `, + }, + + { + code: ` + + + foo + + + `, + }, + + { + code: ` + + {foo && foo} + + `, + }, + + { + code: ` + + {foo ? foo : bar} + + `, + }, + + { + code: ` + + foo + + `, + }, + + { + code: ` + + + foo + + + `, + }, + + { + code: ` + + {bar} + + `, + }, + + { + code: ` + + {bar} + + `, + }, + + { + code: ` + + foo {bar} + + `, + }, + + { + code: ` + + + foo + + + `, + }, + + { + code: ` + + + {bar} + + + `, + }, + + { + code: ` + + + foo {bar} + + + `, + }, + + { + code: ` + + + foo + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo : bar +}> + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo +}> + + + `, + }, + + { + code: ` +foo : bar +}> + + + `, + }, + ], + + invalid: [ + { + code: ` + + `, + errors: 1, + }, + + { + code: ` + + foo + + `, + errors: 1, + }, + + { + code: ` + + <> + foo + + + `, + errors: 1, + }, + + { + code: ` + + + foo + + + `, + errors: 1, + }, + + { + code: ` + + {foo && foo} + + `, + errors: 1, + }, + + { + code: ` + + {foo ? foo : bar} + + `, + errors: 2, + }, + + { + code: ` + + + foo + + + `, + errors: 1, + }, + + { + code: ` + + foo {bar} + + `, + errors: 1, + }, + + { + code: ` + + + foo + + + `, + errors: 1, + }, + + { + code: ` + + + foo + + + `, + errors: 1, + }, + + { + code: ` +foo +}> + + + `, + errors: 1, + }, + + { + code: ` +foo +}> + + + `, + errors: 1, + }, + + { + code: ` +foo : bar +}> + + + `, + errors: 2, + }, + + { + code: ` +foo +}> + + + `, + errors: 1, + }, + ], + } + + // For easier local testing + if (!process.env.CI) { + let only = [] + let skipped = [] + ;[...tests.valid, ...tests.invalid].forEach(t => { + if (t.skip) { + delete t.skip + skipped.push(t) + } + if (t.only) { + delete t.only + only.push(t) + } + }) + const predicate = t => { + if (only.length > 0) { + return only.indexOf(t) !== -1 + } + if (skipped.length > 0) { + return skipped.indexOf(t) === -1 + } + return true + } + tests.valid = tests.valid.filter(predicate) + tests.invalid = tests.invalid.filter(predicate) + } + ruleTester.run('avoid-unwrapped-text', avoidUnwrappedText, tests) +}) diff --git a/eslint/avoid-unwrapped-text.js b/eslint/avoid-unwrapped-text.js new file mode 100644 index 00000000..c9e72386 --- /dev/null +++ b/eslint/avoid-unwrapped-text.js @@ -0,0 +1,111 @@ +'use strict' + +// Partially based on eslint-plugin-react-native. +// Portions of code by Alex Zhukov, MIT license. + +function hasOnlyLineBreak(value) { + return /^[\r\n\t\f\v]+$/.test(value.replace(/ /g, '')) +} + +function getTagName(node) { + const reversedIdentifiers = [] + if ( + node.type === 'JSXElement' && + node.openingElement.type === 'JSXOpeningElement' + ) { + let object = node.openingElement.name + while (object.type === 'JSXMemberExpression') { + if (object.property.type === 'JSXIdentifier') { + reversedIdentifiers.push(object.property.name) + } + object = object.object + } + + if (object.type === 'JSXIdentifier') { + reversedIdentifiers.push(object.name) + } + } + + return reversedIdentifiers.reverse().join('.') +} + +exports.create = function create(context) { + const options = context.options[0] || {} + const impliedTextProps = options.impliedTextProps ?? [] + const impliedTextComponents = options.impliedTextComponents ?? [] + const textProps = [...impliedTextProps] + const textComponents = ['Text', ...impliedTextComponents] + return { + JSXText(node) { + if (typeof node.value !== 'string' || hasOnlyLineBreak(node.value)) { + return + } + let parent = node.parent + while (parent) { + if (parent.type === 'JSXElement') { + const tagName = getTagName(parent) + if (textComponents.includes(tagName) || tagName.endsWith('Text')) { + // We're good. + return + } + if (tagName === 'Trans') { + // Skip over it and check above. + // TODO: Maybe validate that it's present. + parent = parent.parent + continue + } + let message = 'Wrap this string in .' + if (tagName !== 'View') { + 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 string 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 + } + }, + } +} diff --git a/eslint/index.js b/eslint/index.js new file mode 100644 index 00000000..daf5bd81 --- /dev/null +++ b/eslint/index.js @@ -0,0 +1,7 @@ +'use strict' + +module.exports = { + rules: { + 'avoid-unwrapped-text': require('./avoid-unwrapped-text'), + }, +} diff --git a/package.json b/package.json index b9ffbe2d..6165d953 100644 --- a/package.json +++ b/package.json @@ -236,6 +236,7 @@ "babel-preset-expo": "^10.0.0", "detox": "^20.14.8", "eslint": "^8.19.0", + "eslint-plugin-bsky-internal": "link:./eslint", "eslint-plugin-detox": "^1.0.0", "eslint-plugin-ft-flow": "^2.0.3", "eslint-plugin-lingui": "^0.2.0", diff --git a/yarn.lock b/yarn.lock index 5f57bfd4..106599fc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11404,6 +11404,10 @@ eslint-module-utils@^2.8.0: dependencies: debug "^3.2.7" +"eslint-plugin-bsky-internal@link:./eslint": + version "0.0.0" + uid "" + eslint-plugin-detox@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/eslint-plugin-detox/-/eslint-plugin-detox-1.0.0.tgz#2d9c0130e8ebc4ced56efb6eeaf0d0f5c163398d"