From 9d75128e34449b65c41993893e789e6510dcd767 Mon Sep 17 00:00:00 2001 From: Trevor Pierce <1Copenut@users.noreply.github.com> Date: Wed, 28 Sep 2022 15:48:54 -0500 Subject: [PATCH] ESLint rule to validate `css` props are declared before`{...spread}` props (#6257) * WIP. First pass at CSS before spread child props ESLint rule. * Refactoring CSS and spread child prop index calls. * Improving the error reporting message. * Renaming file for clarity. Adding first passing unit tests. * Focused on JSXElement, added unit tests, fixed failures in local linting." * Updating unit tests to pass before renaming. * Registering ESLint rule locally before adding unit tests. * Switched to JSXElement, and array of arrays. * Simplified logic, added unit tests, fixed failures. * Renamed temp arrays and refactored early returns for clarity. * Removing contentProps.css from cssContentStyles array. --- .eslintplugin.js | 1 + .eslintrc.js | 1 + .../eslint-plugin/css_before_spread_props.js | 48 ++++ .../css_before_spread_props.test.js | 245 ++++++++++++++++++ src/components/flyout/flyout_body.tsx | 2 +- src/components/flyout/flyout_footer.tsx | 2 +- src/components/flyout/flyout_header.tsx | 2 +- .../image/image_fullscreen_wrapper.tsx | 2 +- .../page/page_section/page_section.tsx | 3 +- src/components/pagination/pagination.tsx | 4 +- src/components/tool_tip/tool_tip_anchor.tsx | 2 +- src/services/theme/clone_element.test.tsx | 2 +- 12 files changed, 304 insertions(+), 10 deletions(-) create mode 100644 scripts/eslint-plugin/css_before_spread_props.js create mode 100644 scripts/eslint-plugin/css_before_spread_props.test.js diff --git a/.eslintplugin.js b/.eslintplugin.js index 50632e51018..348def29c45 100644 --- a/.eslintplugin.js +++ b/.eslintplugin.js @@ -4,4 +4,5 @@ exports.rules = { 'require-license-header': require('./scripts/eslint-plugin/require_license_header'), 'forward-ref': require('./scripts/eslint-plugin/forward_ref_display_name'), 'css-logical-properties': require('./scripts/eslint-plugin/css_logical_properties'), + 'css_before_spread_props': require('./scripts/eslint-plugin/css_before_spread_props'), }; diff --git a/.eslintrc.js b/.eslintrc.js index f3e0830b119..a366039b97e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -45,6 +45,7 @@ module.exports = { 'local/href-with-rel': 'error', 'local/forward-ref': 'error', 'local/css-logical-properties': 'error', + 'local/css_before_spread_props': 'error', 'local/require-license-header': [ 'warn', { diff --git a/scripts/eslint-plugin/css_before_spread_props.js b/scripts/eslint-plugin/css_before_spread_props.js new file mode 100644 index 00000000000..033695510a5 --- /dev/null +++ b/scripts/eslint-plugin/css_before_spread_props.js @@ -0,0 +1,48 @@ +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'Enforce CSS props are declared before spread props', + }, + }, + create: function (context) { + const allElementsArr = []; + + return { + JSXElement(node) { + const attributesArr = []; + node.openingElement.attributes.forEach((attribute) => + attributesArr.push(attribute) + ); + allElementsArr.push(attributesArr); + }, + 'Program:exit'() { + allElementsArr.forEach((elementArr) => { + const cssPropsIndex = elementArr.findIndex( + (node) => node.name && node.name.name === 'css' + ); + if (cssPropsIndex === -1) return; + + const spreadPropsIndex = elementArr.findIndex( + (node) => node.type === 'JSXSpreadAttribute' + ); + if (spreadPropsIndex === -1) return; + + if (cssPropsIndex > spreadPropsIndex) { + context.report({ + loc: { + line: elementArr[cssPropsIndex].loc.start.line, + column: elementArr[cssPropsIndex].loc.start.column, + }, + message: + '{{ identifier }}: CSS props must be declared before spread props.', + data: { + identifier: elementArr[spreadPropsIndex].argument.name, + }, + }); + } + }); + }, + }; + }, +}; diff --git a/scripts/eslint-plugin/css_before_spread_props.test.js b/scripts/eslint-plugin/css_before_spread_props.test.js new file mode 100644 index 00000000000..1df9860ebce --- /dev/null +++ b/scripts/eslint-plugin/css_before_spread_props.test.js @@ -0,0 +1,245 @@ +import rule from './css_before_spread_props.js'; +const RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parser: require.resolve('babel-eslint'), +}); + +const valid = [ + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + `, + }, + { + code: ` + `, + }, + { + code: ` + `, + }, + { + code: ` + {text} + + `, + }, + { + code: ` + Text string + + `, + }, +]; + +const invalid = [ + { + code: ` + {buttonIcon} + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + {text} + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + Text string + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: `return !href && !onClick ? ( + + + {text} + + + ) : ( + + {text} + + ); + `, + errors: [ + { + message: 'restTextColor: CSS props must be declared before spread props.', + }, + { + message: 'restLink: CSS props must be declared before spread props.', + }, + ], + }, + { + code: `return !href && !onClick ? ( + + + {text} + + + ) : ( + + {text} + + ); + `, + errors: [ + { + message: 'restSpanProps: CSS props must be declared before spread props.', + }, + ], + }, +] + +ruleTester.run('css_before_spread_props', rule, { + valid, + invalid, +}); diff --git a/src/components/flyout/flyout_body.tsx b/src/components/flyout/flyout_body.tsx index 097231d3f3e..d17bf1274fc 100644 --- a/src/components/flyout/flyout_body.tsx +++ b/src/components/flyout/flyout_body.tsx @@ -42,7 +42,7 @@ export const EuiFlyoutBody: EuiFlyoutBodyProps = ({ ]; return ( -
+
+
{children}
); diff --git a/src/components/flyout/flyout_header.tsx b/src/components/flyout/flyout_header.tsx index c3833797036..9323b6e1ff7 100644 --- a/src/components/flyout/flyout_header.tsx +++ b/src/components/flyout/flyout_header.tsx @@ -33,7 +33,7 @@ export const EuiFlyoutHeader: EuiFlyoutHeaderProps = ({ const cssStyles = [styles.euiFlyoutHeader, hasBorder && styles.hasBorder]; return ( -
+
{children}
); diff --git a/src/components/image/image_fullscreen_wrapper.tsx b/src/components/image/image_fullscreen_wrapper.tsx index e31ed0d3960..b94fa02fe51 100644 --- a/src/components/image/image_fullscreen_wrapper.tsx +++ b/src/components/image/image_fullscreen_wrapper.tsx @@ -70,9 +70,9 @@ export const EuiImageFullScreenWrapper: FunctionComponent <>
= ({ bottomBorder === true && styles.border, alignment.toLowerCase().includes('center') && contentStyles.center, restrictWidth && contentStyles.restrictWidth, - contentProps?.css && contentProps.css, ]; return ( -
+
{children}
diff --git a/src/components/pagination/pagination.tsx b/src/components/pagination/pagination.tsx index a12622e2f3e..17332fc6fd0 100644 --- a/src/components/pagination/pagination.tsx +++ b/src/components/pagination/pagination.tsx @@ -292,9 +292,9 @@ export const EuiPagination: FunctionComponent = ({ centerPageCount = (
    {firstPageButtons} {selectablePages} diff --git a/src/components/tool_tip/tool_tip_anchor.tsx b/src/components/tool_tip/tool_tip_anchor.tsx index 7429e3694c1..bc4708f73fa 100644 --- a/src/components/tool_tip/tool_tip_anchor.tsx +++ b/src/components/tool_tip/tool_tip_anchor.tsx @@ -56,8 +56,8 @@ export const EuiToolTipAnchor = forwardRef< // eslint-disable-next-line jsx-a11y/mouse-events-have-key-events { it('handles components', () => { const TestComponent: React.FC = (props) => ( -
    +
    hello world
    );