From f7adc25362a198943554c9c1c4751cf47e53f5d5 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 10 Nov 2021 15:24:56 -0700 Subject: [PATCH 1/8] WIP --- .eslintplugin.js | 1 + .eslintrc.js | 1 + scripts/eslint-plugin/copy_in_jsx.js | 68 +++++++++++++++++++ scripts/eslint-plugin/copy_in_jsx.test.js | 49 +++++++++++++ .../forward_ref_display_name.test.js | 2 +- scripts/eslint-plugin/rel.test.js | 2 +- src-docs/.eslintrc.js | 1 + 7 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 scripts/eslint-plugin/copy_in_jsx.js create mode 100644 scripts/eslint-plugin/copy_in_jsx.test.js diff --git a/.eslintplugin.js b/.eslintplugin.js index 2b3949ff08f..8b682e2daa6 100644 --- a/.eslintplugin.js +++ b/.eslintplugin.js @@ -1,4 +1,5 @@ exports.rules = { + 'copy-in-jsx': require('./scripts/eslint-plugin/copy_in_jsx'), i18n: require('./scripts/eslint-plugin/i18n'), 'href-with-rel': require('./scripts/eslint-plugin/rel'), 'require-license-header': require('./scripts/eslint-plugin/require_license_header'), diff --git a/.eslintrc.js b/.eslintrc.js index 5acc6b7ebc4..751691d6972 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -40,6 +40,7 @@ module.exports = { plugins: ['jsx-a11y', 'prettier', 'local', 'react-hooks', '@emotion'], rules: { 'prefer-template': 'error', + 'local/copy-in-jsx': 'error', 'local/i18n': 'error', 'local/href-with-rel': 'error', 'local/forward-ref': 'error', diff --git a/scripts/eslint-plugin/copy_in_jsx.js b/scripts/eslint-plugin/copy_in_jsx.js new file mode 100644 index 00000000000..0e8c9416de9 --- /dev/null +++ b/scripts/eslint-plugin/copy_in_jsx.js @@ -0,0 +1,68 @@ +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'Prevent hard-coded copy in JSX tag children content', + }, + }, + create: function(context) { + return { + /** + * Props of any component is defined in ArrowFunctions + * Example: const EuiButton = ({ foo, bar }) => {}; + */ + JSXElement(node) { + /* + context.report({ + node: objectPattern.properties[href], + message: 'Props must contain rel if href is defined', + }); + */ + const childrenAttribute = node.openingElement.attributes.find(({ name }) => name.name === 'children'); + + let children = node.children.length > 0 ? node.children : childrenAttribute?.value; + if (children == null) return; // nothing to validate + + if (Array.isArray(children)) { + children.forEach(child => validateChild(context, child)) + } else { + validateChild(context, children); + } + }, + }; + }, +}; + +function validateChild(context, child) { + switch (child.type) { + // validate each item in the array + case 'ArrayExpression': + child.elements.forEach(element => validateChild(context, element)); + break; + + // always okay as a child + case 'JSXElement': + break; + + // validate its expression + case 'JSXExpressionContainer': + validateChild(context, child.expression); + break; + + // literals are prevented + case 'Literal': + context.report({ + node: child, + message: 'Literal values are not allowed as JSX children', + }); + break; + + // fail on unknown types + default: + context.report({ + node: child, + message: `Unable to process JSX child of type "${child.type}"`, + }); + break; + } +} diff --git a/scripts/eslint-plugin/copy_in_jsx.test.js b/scripts/eslint-plugin/copy_in_jsx.test.js new file mode 100644 index 00000000000..a1a2488d74f --- /dev/null +++ b/scripts/eslint-plugin/copy_in_jsx.test.js @@ -0,0 +1,49 @@ +const rule = require('./copy_in_jsx'); +const RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parser: require.resolve('babel-eslint'), +}); + +const valid = [ + // no children + '', // with closing tag + + // one child + '', // JSX child + '', // like React, prefer JSX children over the attribute + + // multiple children + '', // JSX children + '', // children attribute + + // variables + 'const value = ; ', +]; + +const invalid = [ + { + code: '', + errors: [ + { + message: 'Literal values are not allowed as JSX children', + }, + ], + }, + { + code: '', + errors: [ + { + message: 'Literal values are not allowed as JSX children', + }, + ], + }, + +]; + +ruleTester.run('copy-in-jsx', rule, { + valid, + invalid, +}); diff --git a/scripts/eslint-plugin/forward_ref_display_name.test.js b/scripts/eslint-plugin/forward_ref_display_name.test.js index 15f840150fa..3ed36f70a8f 100644 --- a/scripts/eslint-plugin/forward_ref_display_name.test.js +++ b/scripts/eslint-plugin/forward_ref_display_name.test.js @@ -2,7 +2,7 @@ import rule from './forward_ref_display_name.js'; const RuleTester = require('eslint').RuleTester; const ruleTester = new RuleTester({ - parser: 'babel-eslint', + parser: require.resolve('babel-eslint'), }); const valid = [ diff --git a/scripts/eslint-plugin/rel.test.js b/scripts/eslint-plugin/rel.test.js index b29599983dd..554dadfa7ae 100644 --- a/scripts/eslint-plugin/rel.test.js +++ b/scripts/eslint-plugin/rel.test.js @@ -2,7 +2,7 @@ const rule = require('./rel'); const RuleTester = require('eslint').RuleTester; const ruleTester = new RuleTester({ - parser: 'babel-eslint', + parser: require.resolve('babel-eslint'), }); const valid = [ diff --git a/src-docs/.eslintrc.js b/src-docs/.eslintrc.js index 0242d922ac5..d18dcb516df 100644 --- a/src-docs/.eslintrc.js +++ b/src-docs/.eslintrc.js @@ -2,6 +2,7 @@ module.exports = { extends: '../.eslintrc.js', rules: { '@typescript-eslint/no-var-requires': 'off', + 'local/copy-in-jsx': 'off', 'local/require-license-header': 'off', }, }; From abf6c5bceb197cfdd4c9117612322ac75e9c7bd4 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 16 Nov 2021 08:28:40 -0700 Subject: [PATCH 2/8] Working lint rule --- .eslintrc.js | 1 - scripts/eslint-plugin/copy_in_jsx.js | 208 +++++++++++++++++- scripts/eslint-plugin/copy_in_jsx.test.js | 140 +++++++++++- scripts/eslint-plugin/fixtures/code.tsx | 0 .../fixtures/tsconfig-eslint.json | 14 ++ src-docs/.eslintrc.js | 1 - src/.eslintrc.js | 24 ++ src/example.tsx | 21 ++ 8 files changed, 389 insertions(+), 20 deletions(-) create mode 100644 scripts/eslint-plugin/fixtures/code.tsx create mode 100644 scripts/eslint-plugin/fixtures/tsconfig-eslint.json create mode 100644 src/.eslintrc.js create mode 100644 src/example.tsx diff --git a/.eslintrc.js b/.eslintrc.js index 751691d6972..5acc6b7ebc4 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -40,7 +40,6 @@ module.exports = { plugins: ['jsx-a11y', 'prettier', 'local', 'react-hooks', '@emotion'], rules: { 'prefer-template': 'error', - 'local/copy-in-jsx': 'error', 'local/i18n': 'error', 'local/href-with-rel': 'error', 'local/forward-ref': 'error', diff --git a/scripts/eslint-plugin/copy_in_jsx.js b/scripts/eslint-plugin/copy_in_jsx.js index 0e8c9416de9..1486a4d8707 100644 --- a/scripts/eslint-plugin/copy_in_jsx.js +++ b/scripts/eslint-plugin/copy_in_jsx.js @@ -1,3 +1,19 @@ +// const { +// TSESTree, +// AST_NODE_TYPES, +// } = require('@typescript-eslint/experimental-utils'); +// const { +// isObjectType, +// isObjectFlagSet, +// isStrictCompilerOptionEnabled, +// isTypeFlagSet, +// isVariableDeclaration, +// } = require('tsutils'); +const ts = require('typescript'); +const { ESLintUtils, getConstrainedTypeAtLocation, ...rest } = require('@typescript-eslint/experimental-utils'); + +const { getParserServices } = ESLintUtils; + module.exports = { meta: { type: 'problem', @@ -12,13 +28,7 @@ module.exports = { * Example: const EuiButton = ({ foo, bar }) => {}; */ JSXElement(node) { - /* - context.report({ - node: objectPattern.properties[href], - message: 'Props must contain rel if href is defined', - }); - */ - const childrenAttribute = node.openingElement.attributes.find(({ name }) => name.name === 'children'); + const childrenAttribute = node.openingElement.attributes.find(({ name, type }) => type === 'JSXAttribute' && name.name === 'children'); let children = node.children.length > 0 ? node.children : childrenAttribute?.value; if (children == null) return; // nothing to validate @@ -33,28 +43,158 @@ module.exports = { }, }; +const allowedTypes = new Set(['ReactNode']); +function isStringType(parserServices, checker, node) { + const stringType = checker.getStringType(); + + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + const tsType = checker.getTypeAtLocation(tsNode); + + const typeAsString = checker.typeToString(tsType); + if (allowedTypes.has(typeAsString)) return false; + + // can't validate if TS doesn't know what's up + if (tsType.intrinsicName === 'error') return; + + // resolve constant string values to the wider `string` type + const tsLiteralType = checker.getBaseTypeOfLiteralType(tsType); + + const hasStringType = !!tsLiteralType?.types?.find(type => checker.getBaseTypeOfLiteralType(type) === stringType); + if (tsLiteralType === stringType || hasStringType) { + return true; + } + + return false; +} + function validateChild(context, child) { switch (child.type) { - // validate each item in the array case 'ArrayExpression': + // validate each item in the array child.elements.forEach(element => validateChild(context, element)); break; + case 'ArrowFunctionExpression': + // render props are fine + break; + + case 'CallExpression': { + // useEuiI18n is the singular exception to functions returning strings + if (child.callee.name === 'useEuiI18n') return; + + const parserServices = getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + if (isStringType(parserServices, checker, child)) { + context.report({ + node: child, + message: 'Functions with a string return type cannot be used within JSX', + }); + } + break; + } + + case 'ConditionalExpression': + // test both results of the condition + validateChild(context, child.consequent); + validateChild(context, child.alternate); + break; + + case 'BinaryExpression': + // @TODO: the usual case encountered here is testing a condition, e.g. + // {myValue === something} + // but these can also appear as + // {myValue + 'someString'} + // which is an invariant + // leaving these edge cases as out of scope for at least the PoC + break; + + case 'Identifier': { + // if this variable comes from a function parameter, we have no control and won't prevent usage + if (isIdentifierSourceSafe(context, child)) return; + + const parserServices = getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + if (isStringType(parserServices, checker, child)) { + context.report({ + node: child, + message: 'String-type variables are not allowed as children within JSX', + }); + } + break; + } + // always okay as a child case 'JSXElement': break; + // can't resolve to anything, always okay (usually these are JSX comments) + case 'JSXEmptyExpression': + break; + + // text as a child node is the primary case we want to prevent + case 'JSXText': + // whitespace characters are acceptable (likely exists for code formatting reasons e.g. prettier) + // also allow escaped entities of the pattern &[a-z]; + if (child.value.match(/^(&[a-z]+;|\s)*$/)) return; + + context.report({ + node: child, + message: 'Text is not allowed as children within JSX, use i18n instead (https://elastic.github.io/eui/#/utilities/i18n)', + }); + break; + // validate its expression case 'JSXExpressionContainer': validateChild(context, child.expression); break; + case 'JSXFragment': + // as safe as can be + break; + // literals are prevented case 'Literal': - context.report({ - node: child, - message: 'Literal values are not allowed as JSX children', - }); + if (typeof child.value === 'string') { + context.report({ + node: child, + message: 'String literals are not allowed within JSX', + }); + } + break; + + case 'LogicalExpression': + // @TODO: it would be trivial to also check for a numeric condition here, e.g.: + //
{pages.length && }
+ // if pages.length is 0, React will render the 0 which is not the desired outcome + // we've hit this a few times before, usually caught in a pull review but not always + validateChild(context, child.left); + validateChild(context, child.right); + break; + + case 'MemberExpression': + // @TODO: currently relying solely on type validation that this isn't a string + // instead of checking the source of the value like we do for an Identifier + + const parserServices = getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + if (isStringType(parserServices, checker, child)) { + context.report({ + node: child, + message: 'String-type variables are not allowed as children within JSX', + }); + } + break; + + case 'SpreadElement': + // treat spreading as safe + break; + + case 'UnaryExpression': + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators#unary_operators + // the only unary operator that returns a string is `typeof`, and if that's used in JSX well then why not break; // fail on unknown types @@ -66,3 +206,47 @@ function validateChild(context, child) { break; } } + +function isIdentifierSourceSafe(context, identifierNode) { + const scope = context.getScope(); + const scopeVariables = scope.set; + const variableScope = scopeVariables.get(identifierNode.name); + + // if we know where this variable comes from, we can't check it + // (this exists mostly for this rule's test cases) + if (variableScope == null) { + return true; + } + + const variableDef = variableScope.defs[0]; // @TODO: consider re-defs? + + // this comes from a function argument, allow + if (variableDef.type === 'Parameter') { + return true; + } + + // allow if this variable came from a call to useEuiI18n + if (variableDef.node.type === 'VariableDeclarator' && variableDef.node.init?.type === 'CallExpression' && variableDef.node.init?.callee.name === 'useEuiI18n') { + return true; + } + + // allow variables that destructure from `this.props` + if (variableDef.node.type === 'VariableDeclarator') { + const { init } = variableDef.node; + if (init?.type === 'MemberExpression') { + const { object, property } = init; + if (object.type === 'ThisExpression' && property.type === 'Identifier' && property.name === 'props') { + return true; + } + } + } + + // KEEP THIS AS THE FINAL CHECK + // we kinda shrug here and say *maybe*, depends on the source of this variable + // must be last, otherwise it interferes with the other checks + if (variableDef.type === 'Variable') { + // can we trace this back to a function argument? + const init = variableDef.node?.init; + return init?.type === 'Identifier' ? isIdentifierSourceSafe(context, init) : false; + } +} diff --git a/scripts/eslint-plugin/copy_in_jsx.test.js b/scripts/eslint-plugin/copy_in_jsx.test.js index a1a2488d74f..5919c46f621 100644 --- a/scripts/eslint-plugin/copy_in_jsx.test.js +++ b/scripts/eslint-plugin/copy_in_jsx.test.js @@ -1,8 +1,14 @@ +const { join } = require('path'); const rule = require('./copy_in_jsx'); const RuleTester = require('eslint').RuleTester; const ruleTester = new RuleTester({ - parser: require.resolve('babel-eslint'), + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + tsconfigRootDir: join(__dirname, 'fixtures'), + project: './tsconfig-eslint.json', + }, }); const valid = [ @@ -19,8 +25,51 @@ const valid = [ '', // JSX children '', // children attribute + // literals + '{5}', // numeric + '{false}', // boolean + // variables - 'const value = ; ', + 'const value = ; ', // variable is not a string + 'function Component({ name }: { name: string }) { return {name} }', // variable comes from an argument + + // CallExpression + '{callback()}', // unknown return type + 'const callback = () => 5; {callback()}', // valid return type + + // useEuiI18n allowances + 'const useEuiI18n: () => ""; const stuff = useEuiI18n(); {stuff}', // used outside of JSX + 'const useEuiI18n: () => ""; {useEuiI18n()}', // used inside of JSX + + // MemberExpression + 'const vals = { numeric: 5 };{vals.numeric}', + + // edge cases + '', // JSXSpreadAttribute + '\n\t ', // whitespace-only characters + '{isTrue ? 1 : 0}', // ConditionalExpression + 'const condition = 5;
{condition && }
', // LogicalExpression + '{value === true ? 1 : 0}', // BinaryExpression + '{/* This is a comment */}', // JSXEmptyExpression + '
…
', // escaped entities + 'const isRead = true; {!isRead}', // UnaryExpression + '
{[...additionalButtons, expandButton]}
', // SpreadElement + '<>', // JSXFragment + ` + function Component(props: { val: string }) { + const { val } = props; + return {val} + } + `, // destructured value + ` + class Component { + props: { stringValue: string } = null; + render() { + const { stringValue } = this.props; + return
{stringValue}
; + } + } + `, // destructured from this.props ]; const invalid = [ @@ -28,7 +77,15 @@ const invalid = [ code: '', errors: [ { - message: 'Literal values are not allowed as JSX children', + message: 'Text is not allowed as children within JSX, use i18n instead (https://elastic.github.io/eui/#/utilities/i18n)', + }, + ], + }, + { + code: '', + errors: [ + { + message: 'String literals are not allowed within JSX', }, ], }, @@ -36,14 +93,85 @@ const invalid = [ code: '', errors: [ { - message: 'Literal values are not allowed as JSX children', + message: 'Text is not allowed as children within JSX, use i18n instead (https://elastic.github.io/eui/#/utilities/i18n)', + }, + ], + }, + { + code: 'const content = "Content"; ', + errors: [ + { + message: 'String-type variables are not allowed as children within JSX', }, ], }, + // CallExpression + { + code: 'const callback = () => "5"; {callback()}', + errors: [ + { + message: 'Functions with a string return type cannot be used within JSX', + }, + ], + }, + { + code: 'const callback = (): number | string => "5"; {callback()}', + errors: [ + { + message: 'Functions with a string return type cannot be used within JSX', + }, + ], + }, + + // MemberExpression + { + code: 'const vals = { string: "" };{vals.string}', + errors: [ + { + message: 'String-type variables are not allowed as children within JSX', + }, + ], + }, + + // edge cases + { + code: 'const text = "zero"; {isTrue ? "one" : text}', + errors: [ + { + message: 'String literals are not allowed within JSX', + }, + { + message: 'String-type variables are not allowed as children within JSX', + }, + ], + }, + { + code: 'const stringThatMightHaveLength = "";
{stringThatMightHaveLength && "Copy"}
', + errors: [ + { + message: 'String-type variables are not allowed as children within JSX', + }, + { + message: 'String literals are not allowed within JSX', + }, + ], + }, ]; ruleTester.run('copy-in-jsx', rule, { - valid, - invalid, + valid: valid.map(insertFilename), + invalid: invalid.map(insertFilename), }); + +function insertFilename(test) { + if (typeof test === 'string') { + return { + code: test, + filename: 'code.tsx' + }; + } else { + test.filename = 'code.tsx'; + return test; + } +} diff --git a/scripts/eslint-plugin/fixtures/code.tsx b/scripts/eslint-plugin/fixtures/code.tsx new file mode 100644 index 00000000000..e69de29bb2d diff --git a/scripts/eslint-plugin/fixtures/tsconfig-eslint.json b/scripts/eslint-plugin/fixtures/tsconfig-eslint.json new file mode 100644 index 00000000000..db74f08493d --- /dev/null +++ b/scripts/eslint-plugin/fixtures/tsconfig-eslint.json @@ -0,0 +1,14 @@ +{ + "compilerOptions": { + "jsx": "preserve", + "target": "es5", + "module": "commonjs", + "strict": true, + "esModuleInterop": true, + "lib": ["es2015", "es2017", "esnext"], + "experimentalDecorators": true + }, + "include": [ + "code.tsx", + ] +} diff --git a/src-docs/.eslintrc.js b/src-docs/.eslintrc.js index d18dcb516df..0242d922ac5 100644 --- a/src-docs/.eslintrc.js +++ b/src-docs/.eslintrc.js @@ -2,7 +2,6 @@ module.exports = { extends: '../.eslintrc.js', rules: { '@typescript-eslint/no-var-requires': 'off', - 'local/copy-in-jsx': 'off', 'local/require-license-header': 'off', }, }; diff --git a/src/.eslintrc.js b/src/.eslintrc.js new file mode 100644 index 00000000000..6a7de07badf --- /dev/null +++ b/src/.eslintrc.js @@ -0,0 +1,24 @@ +const { join } = require('path'); + +module.exports = { + extends: '../.eslintrc.js', + parserOptions: { + sourceType: 'module', + tsconfigRootDir: join(__dirname, '..'), + project: './tsconfig.json', + }, + overrides: [ + { + files: ['*.{ts,tsx}'], + rules: { + 'local/copy-in-jsx': 'error', + }, + }, + { + files: ['*.{test,spec}.tsx'], + rules: { + 'local/copy-in-jsx': 'off', + }, + }, + ], +}; diff --git a/src/example.tsx b/src/example.tsx new file mode 100644 index 00000000000..d5ac3043acb --- /dev/null +++ b/src/example.tsx @@ -0,0 +1,21 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import React, { FunctionComponent } from 'react'; + +interface ExampleProps { + name: string; + age: number; +} +export const Example: FunctionComponent = ({ name, age }) => { + return ( +
+ My name is {name} and I am {age} years old. +
+ ); +}; From 8dd35a2fc1fac93f6b699d3e6b11190a0ae550c0 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 16 Nov 2021 12:19:45 -0700 Subject: [PATCH 3/8] rename copy-in-jsx -> text-in-jsx --- .eslintplugin.js | 2 +- .../{copy_in_jsx.js => text_in_jsx.js} | 0 ...opy_in_jsx.test.js => text_in_jsx.test.js} | 4 ++-- src/.eslintrc.js | 4 ++-- src/example.tsx | 21 ------------------- 5 files changed, 5 insertions(+), 26 deletions(-) rename scripts/eslint-plugin/{copy_in_jsx.js => text_in_jsx.js} (100%) rename scripts/eslint-plugin/{copy_in_jsx.test.js => text_in_jsx.test.js} (98%) delete mode 100644 src/example.tsx diff --git a/.eslintplugin.js b/.eslintplugin.js index 8b682e2daa6..29ef940abb5 100644 --- a/.eslintplugin.js +++ b/.eslintplugin.js @@ -1,5 +1,5 @@ exports.rules = { - 'copy-in-jsx': require('./scripts/eslint-plugin/copy_in_jsx'), + 'text-in-jsx': require('./scripts/eslint-plugin/text_in_jsx'), i18n: require('./scripts/eslint-plugin/i18n'), 'href-with-rel': require('./scripts/eslint-plugin/rel'), 'require-license-header': require('./scripts/eslint-plugin/require_license_header'), diff --git a/scripts/eslint-plugin/copy_in_jsx.js b/scripts/eslint-plugin/text_in_jsx.js similarity index 100% rename from scripts/eslint-plugin/copy_in_jsx.js rename to scripts/eslint-plugin/text_in_jsx.js diff --git a/scripts/eslint-plugin/copy_in_jsx.test.js b/scripts/eslint-plugin/text_in_jsx.test.js similarity index 98% rename from scripts/eslint-plugin/copy_in_jsx.test.js rename to scripts/eslint-plugin/text_in_jsx.test.js index 5919c46f621..9eb1b1d2585 100644 --- a/scripts/eslint-plugin/copy_in_jsx.test.js +++ b/scripts/eslint-plugin/text_in_jsx.test.js @@ -1,5 +1,5 @@ const { join } = require('path'); -const rule = require('./copy_in_jsx'); +const rule = require('./text_in_jsx'); const RuleTester = require('eslint').RuleTester; const ruleTester = new RuleTester({ @@ -159,7 +159,7 @@ const invalid = [ }, ]; -ruleTester.run('copy-in-jsx', rule, { +ruleTester.run('text-in-jsx', rule, { valid: valid.map(insertFilename), invalid: invalid.map(insertFilename), }); diff --git a/src/.eslintrc.js b/src/.eslintrc.js index 6a7de07badf..448fb2e4bf0 100644 --- a/src/.eslintrc.js +++ b/src/.eslintrc.js @@ -11,13 +11,13 @@ module.exports = { { files: ['*.{ts,tsx}'], rules: { - 'local/copy-in-jsx': 'error', + 'local/text-in-jsx': 'error', }, }, { files: ['*.{test,spec}.tsx'], rules: { - 'local/copy-in-jsx': 'off', + 'local/text-in-jsx': 'off', }, }, ], diff --git a/src/example.tsx b/src/example.tsx deleted file mode 100644 index d5ac3043acb..00000000000 --- a/src/example.tsx +++ /dev/null @@ -1,21 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -import React, { FunctionComponent } from 'react'; - -interface ExampleProps { - name: string; - age: number; -} -export const Example: FunctionComponent = ({ name, age }) => { - return ( -
- My name is {name} and I am {age} years old. -
- ); -}; From fb684b7c08f9ee25b49de702bb3867160c47c14c Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 18 Nov 2021 15:45:25 -0700 Subject: [PATCH 4/8] Some cleanup --- .eslintignore | 1 + scripts/eslint-plugin/text_in_jsx.js | 6 ++++-- scripts/eslint-plugin/text_in_jsx.test.js | 3 ++- src/.eslintrc.js | 10 +++++----- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/.eslintignore b/.eslintignore index 507addcbe82..a326494d7c3 100644 --- a/.eslintignore +++ b/.eslintignore @@ -11,3 +11,4 @@ packages scripts generator-eui cypress +*.d.ts diff --git a/scripts/eslint-plugin/text_in_jsx.js b/scripts/eslint-plugin/text_in_jsx.js index 1486a4d8707..1f43daf7e2c 100644 --- a/scripts/eslint-plugin/text_in_jsx.js +++ b/scripts/eslint-plugin/text_in_jsx.js @@ -174,8 +174,10 @@ function validateChild(context, child) { break; case 'MemberExpression': - // @TODO: currently relying solely on type validation that this isn't a string - // instead of checking the source of the value like we do for an Identifier + // if the top-level object of this expression is a safe identifier, treat this as okay + let objectIdentifier = child.object; + while (objectIdentifier.type === 'MemberExpression') objectIdentifier = objectIdentifier.object; + if (isIdentifierSourceSafe(context, objectIdentifier)) return; const parserServices = getParserServices(context); const checker = parserServices.program.getTypeChecker(); diff --git a/scripts/eslint-plugin/text_in_jsx.test.js b/scripts/eslint-plugin/text_in_jsx.test.js index 9eb1b1d2585..622d9de9ce9 100644 --- a/scripts/eslint-plugin/text_in_jsx.test.js +++ b/scripts/eslint-plugin/text_in_jsx.test.js @@ -42,7 +42,8 @@ const valid = [ 'const useEuiI18n: () => ""; {useEuiI18n()}', // used inside of JSX // MemberExpression - 'const vals = { numeric: 5 };{vals.numeric}', + 'const vals = { numeric: 5 };{vals.numeric}', // isn't a string + 'function Component({ person }: { person: { name: string } }) { return {person.name} }', // variable comes from an argument // edge cases '', // JSXSpreadAttribute diff --git a/src/.eslintrc.js b/src/.eslintrc.js index 448fb2e4bf0..b43b0fa16ca 100644 --- a/src/.eslintrc.js +++ b/src/.eslintrc.js @@ -2,14 +2,14 @@ const { join } = require('path'); module.exports = { extends: '../.eslintrc.js', - parserOptions: { - sourceType: 'module', - tsconfigRootDir: join(__dirname, '..'), - project: './tsconfig.json', - }, overrides: [ { files: ['*.{ts,tsx}'], + parserOptions: { + sourceType: 'module', + tsconfigRootDir: join(__dirname, '..'), + project: './tsconfig.json', + }, rules: { 'local/text-in-jsx': 'error', }, From 17ce222bde27da657af2898663c616fd9c7aedc8 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 19 Nov 2021 10:51:38 -0700 Subject: [PATCH 5/8] handle TemplateLiteral syntax --- scripts/eslint-plugin/text_in_jsx.js | 7 +++++++ scripts/eslint-plugin/text_in_jsx.test.js | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/scripts/eslint-plugin/text_in_jsx.js b/scripts/eslint-plugin/text_in_jsx.js index 1f43daf7e2c..8cafec868be 100644 --- a/scripts/eslint-plugin/text_in_jsx.js +++ b/scripts/eslint-plugin/text_in_jsx.js @@ -194,6 +194,13 @@ function validateChild(context, child) { // treat spreading as safe break; + case 'TemplateLiteral': + context.report({ + node: child, + message: 'String-type variables are not allowed as children within JSX', + }); + break; + case 'UnaryExpression': // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators#unary_operators // the only unary operator that returns a string is `typeof`, and if that's used in JSX well then why not diff --git a/scripts/eslint-plugin/text_in_jsx.test.js b/scripts/eslint-plugin/text_in_jsx.test.js index 622d9de9ce9..e31f190f5a2 100644 --- a/scripts/eslint-plugin/text_in_jsx.test.js +++ b/scripts/eslint-plugin/text_in_jsx.test.js @@ -135,6 +135,16 @@ const invalid = [ ], }, + // TemplateLiteral + { + code: '{`content`}', + errors: [ + { + message: 'String-type variables are not allowed as children within JSX', + }, + ], + }, + // edge cases { code: 'const text = "zero"; {isTrue ? "one" : text}', From b456f0954a582e2803ff4748f5267f8b575bf5e7 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 19 Nov 2021 11:32:01 -0700 Subject: [PATCH 6/8] handle ChainExpression syntax --- scripts/eslint-plugin/text_in_jsx.js | 12 ++++++++++++ scripts/eslint-plugin/text_in_jsx.test.js | 13 +++++++++++++ 2 files changed, 25 insertions(+) diff --git a/scripts/eslint-plugin/text_in_jsx.js b/scripts/eslint-plugin/text_in_jsx.js index 8cafec868be..235d1d205a3 100644 --- a/scripts/eslint-plugin/text_in_jsx.js +++ b/scripts/eslint-plugin/text_in_jsx.js @@ -109,6 +109,18 @@ function validateChild(context, child) { // leaving these edge cases as out of scope for at least the PoC break; + case 'ChainExpression': + // Wraps a MemberExpression, indicating that the property access is chained with ?. + if (child.expression.type !== 'MemberExpression') { + context.report({ + node: child, + message: `Unable to process a chained expression of type "${child.expression.type}"`, + }); + } else { + validateChild(context, child.expression); + } + break; + case 'Identifier': { // if this variable comes from a function parameter, we have no control and won't prevent usage if (isIdentifierSourceSafe(context, child)) return; diff --git a/scripts/eslint-plugin/text_in_jsx.test.js b/scripts/eslint-plugin/text_in_jsx.test.js index e31f190f5a2..9b824eb8317 100644 --- a/scripts/eslint-plugin/text_in_jsx.test.js +++ b/scripts/eslint-plugin/text_in_jsx.test.js @@ -45,6 +45,9 @@ const valid = [ 'const vals = { numeric: 5 };{vals.numeric}', // isn't a string 'function Component({ person }: { person: { name: string } }) { return {person.name} }', // variable comes from an argument + // ChainExpression + 'const values = { numeric: 5 }; {values?.numeric}', // isn't a string + // edge cases '', // JSXSpreadAttribute '\n\t ', // whitespace-only characters @@ -135,6 +138,16 @@ const invalid = [ ], }, + // ChainExpression + { + code: 'const vals = { string: "" };{vals?.string}', + errors: [ + { + message: 'String-type variables are not allowed as children within JSX', + }, + ], + }, + // TemplateLiteral { code: '{`content`}', From 676d77dc18937010cfaa16fd6dcad64a1a755568 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 19 Nov 2021 14:59:49 -0700 Subject: [PATCH 7/8] don't lint .eslintrc files --- .eslintignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.eslintignore b/.eslintignore index a326494d7c3..fe7e134ca19 100644 --- a/.eslintignore +++ b/.eslintignore @@ -12,3 +12,4 @@ scripts generator-eui cypress *.d.ts +.eslintrc.js From 3a8837be6f20e9a884cd03e551b047b2e2c2496b Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Mon, 22 Nov 2021 09:28:59 -0700 Subject: [PATCH 8/8] Ignore test files when linting for text-in-jsx --- src/.eslintrc.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/.eslintrc.js b/src/.eslintrc.js index b43b0fa16ca..b31d557701f 100644 --- a/src/.eslintrc.js +++ b/src/.eslintrc.js @@ -5,6 +5,7 @@ module.exports = { overrides: [ { files: ['*.{ts,tsx}'], + excludedFiles: ['*.@(spec|test).{ts,tsx}'], parserOptions: { sourceType: 'module', tsconfigRootDir: join(__dirname, '..'), @@ -14,11 +15,5 @@ module.exports = { 'local/text-in-jsx': 'error', }, }, - { - files: ['*.{test,spec}.tsx'], - rules: { - 'local/text-in-jsx': 'off', - }, - }, ], };