From 70a949dddee0c7d82a68449f6fdbbc43bf5ed673 Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Sat, 9 May 2020 19:39:26 +0530 Subject: [PATCH 1/8] Added es lint rule to ensure display name for forwardRef components --- .eslintplugin.js | 1 + .eslintrc.js | 1 + .../eslint-plugin/forward_ref_display_name.js | 78 +++++++++++++++++++ .../forward_ref_display_name.test.js | 47 +++++++++++ 4 files changed, 127 insertions(+) create mode 100644 scripts/eslint-plugin/forward_ref_display_name.js create mode 100644 scripts/eslint-plugin/forward_ref_display_name.test.js diff --git a/.eslintplugin.js b/.eslintplugin.js index 98ae781dd92..2b3949ff08f 100644 --- a/.eslintplugin.js +++ b/.eslintplugin.js @@ -2,4 +2,5 @@ exports.rules = { i18n: require('./scripts/eslint-plugin/i18n'), 'href-with-rel': require('./scripts/eslint-plugin/rel'), 'require-license-header': require('./scripts/eslint-plugin/require_license_header'), + 'forward-ref': require('./scripts/eslint-plugin/forward_ref_display_name'), }; diff --git a/.eslintrc.js b/.eslintrc.js index e6390e7d5fa..ebdbaa72c6f 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -58,6 +58,7 @@ module.exports = { "prefer-template": "error", "local/i18n": "error", "local/href-with-rel": "error", + "local/forward-ref": "error", "local/require-license-header": [ 'warn', { diff --git a/scripts/eslint-plugin/forward_ref_display_name.js b/scripts/eslint-plugin/forward_ref_display_name.js new file mode 100644 index 00000000000..4c11287ef4b --- /dev/null +++ b/scripts/eslint-plugin/forward_ref_display_name.js @@ -0,0 +1,78 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'Enforce display name', + }, + }, + create: function(context) { + const forwardRefUsages = []; + const displayNameUsages = []; + return { + VariableDeclarator(node) { + if (node.init && node.init.type === 'CallExpression') { + if ( + node.init.callee && + node.init.callee.type === 'MemberExpression' + ) { + if ( + node.init.callee.property && + node.init.callee.property.name === 'forwardRef' + ) { + forwardRefUsages.push(node.id); + } + } + if (node.init.callee && node.init.callee.name === 'forwardRef') { + forwardRefUsages.push(node.id); + } + } + }, + Identifier(node) { + if (node.name === 'displayName') { + displayNameUsages.push(node.parent.object); + } + }, + 'Program:exit'() { + forwardRefUsages.forEach(identifier => { + if (!isDisplayNameUsed(identifier)) { + context.report({ + node: identifier, + message: 'Forward refs must use display name', + fix: function(fixer) { + fixer.insertTextAfter( + identifier, + `${identifier.name}.displayName = ${identifier.name}` + ); + }, + }); + } + }); + }, + }; + function isDisplayNameUsed(identifier) { + const node = displayNameUsages.find( + displayName => displayName.name === identifier.name + ); + return !!node; + } + }, +}; diff --git a/scripts/eslint-plugin/forward_ref_display_name.test.js b/scripts/eslint-plugin/forward_ref_display_name.test.js new file mode 100644 index 00000000000..fbae57eebc4 --- /dev/null +++ b/scripts/eslint-plugin/forward_ref_display_name.test.js @@ -0,0 +1,47 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import rule from './forward_ref_display_name.js'; +const RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parser: 'babel-eslint', +}); + +const valid = [ + `const Component = React.forwardRef(() => {}) + Component.displayName = "EuiBadgeGroup" +`, +]; + +const invalid = [ + { + code: 'const Component = React.forwardRef(() => {})', + errors: [ + { + message: 'You must use display name', + }, + ], + }, +]; + +ruleTester.run('forward_ref_display_name', rule, { + valid, + invalid, +}); From 64df4e4cba2669ee712da92d6067b72b8cdb590d Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Sat, 9 May 2020 19:42:35 +0530 Subject: [PATCH 2/8] Added display name for `forwardRef` components --- src/components/color_picker/color_picker_swatch.tsx | 2 ++ src/components/color_picker/saturation.tsx | 2 ++ src/components/datagrid/data_grid_header_row.tsx | 2 ++ src/components/form/range/range_slider.tsx | 2 ++ src/components/form/range/range_wrapper.tsx | 2 ++ 5 files changed, 10 insertions(+) diff --git a/src/components/color_picker/color_picker_swatch.tsx b/src/components/color_picker/color_picker_swatch.tsx index fa3d537eebb..a3d0f6edf67 100644 --- a/src/components/color_picker/color_picker_swatch.tsx +++ b/src/components/color_picker/color_picker_swatch.tsx @@ -53,3 +53,5 @@ export const EuiColorPickerSwatch = forwardRef< /> ); }); + +EuiColorPickerSwatch.displayName = 'EuiColorPickerSwatch'; diff --git a/src/components/color_picker/saturation.tsx b/src/components/color_picker/saturation.tsx index 20f491858fe..add0b5cc1ee 100644 --- a/src/components/color_picker/saturation.tsx +++ b/src/components/color_picker/saturation.tsx @@ -208,3 +208,5 @@ export const EuiSaturation = forwardRef( ); } ); + +EuiSaturation.displayName = 'EuiSaturation'; diff --git a/src/components/datagrid/data_grid_header_row.tsx b/src/components/datagrid/data_grid_header_row.tsx index a3138776189..8457c1345d0 100644 --- a/src/components/datagrid/data_grid_header_row.tsx +++ b/src/components/datagrid/data_grid_header_row.tsx @@ -122,4 +122,6 @@ const EuiDataGridHeaderRow = forwardRef< ); }); +EuiDataGridHeaderRow.displayName = 'EuiDataGridHeaderRow'; + export { EuiDataGridHeaderRow }; diff --git a/src/components/form/range/range_slider.tsx b/src/components/form/range/range_slider.tsx index e20166d4469..e524dbe42f8 100644 --- a/src/components/form/range/range_slider.tsx +++ b/src/components/form/range/range_slider.tsx @@ -94,3 +94,5 @@ export const EuiRangeSlider = forwardRef( ); } ); + +EuiRangeSlider.displayName = 'EuiRangeSlider'; diff --git a/src/components/form/range/range_wrapper.tsx b/src/components/form/range/range_wrapper.tsx index 8ec6186eb83..f0a1622f9e7 100644 --- a/src/components/form/range/range_wrapper.tsx +++ b/src/components/form/range/range_wrapper.tsx @@ -46,3 +46,5 @@ export const EuiRangeWrapper = forwardRef( ); } ); + +EuiRangeWrapper.displayName = 'EuiRangeWrapper'; From ed0e818000677189957cf819a8689846bde7f677 Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Sat, 9 May 2020 19:53:12 +0530 Subject: [PATCH 3/8] removed fix --- scripts/eslint-plugin/forward_ref_display_name.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/scripts/eslint-plugin/forward_ref_display_name.js b/scripts/eslint-plugin/forward_ref_display_name.js index 4c11287ef4b..b1c760448c0 100644 --- a/scripts/eslint-plugin/forward_ref_display_name.js +++ b/scripts/eslint-plugin/forward_ref_display_name.js @@ -21,7 +21,7 @@ module.exports = { meta: { type: 'problem', docs: { - description: 'Enforce display name', + description: 'Enforce display name to forwardRef components', }, }, create: function(context) { @@ -56,13 +56,7 @@ module.exports = { if (!isDisplayNameUsed(identifier)) { context.report({ node: identifier, - message: 'Forward refs must use display name', - fix: function(fixer) { - fixer.insertTextAfter( - identifier, - `${identifier.name}.displayName = ${identifier.name}` - ); - }, + message: 'Forward ref components must use a display name', }); } }); From 2300364efe690431edd8f28a9cc0c0a979092349 Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Sat, 9 May 2020 20:01:49 +0530 Subject: [PATCH 4/8] Added cl --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 182d278a509..89d90cf72d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## [`master`](https://github.com/elastic/eui/tree/master) +- Added eslint rule to detect `forwardRef` components without `displayName` ([#3451](https://github.com/elastic/eui/pull/3451)) - Added `sortBy` and `sortShift` props to `euiPaletteColorBlind()` for sorting along the color wheel ([#3387](https://github.com/elastic/eui/pull/3387)) - Added `partition` key to `EuiChartThemeType` for Partition chart support ([#3387](https://github.com/elastic/eui/pull/3387)) - Updated `EuiImage`'s `caption` prop type from `string` to `ReactNode` ([#3387](https://github.com/elastic/eui/pull/3387)) From f10d7109e41b82206addc4184a2be247f6784b0f Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Sun, 10 May 2020 21:57:32 +0530 Subject: [PATCH 5/8] Updated message in tests --- scripts/eslint-plugin/forward_ref_display_name.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/eslint-plugin/forward_ref_display_name.test.js b/scripts/eslint-plugin/forward_ref_display_name.test.js index fbae57eebc4..fe1084fcfce 100644 --- a/scripts/eslint-plugin/forward_ref_display_name.test.js +++ b/scripts/eslint-plugin/forward_ref_display_name.test.js @@ -35,7 +35,7 @@ const invalid = [ code: 'const Component = React.forwardRef(() => {})', errors: [ { - message: 'You must use display name', + message: 'Forward ref components must use a display name', }, ], }, From 682e91289eecd6d4aa86324beeebfe82d76ce23e Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Wed, 13 May 2020 20:55:30 +0530 Subject: [PATCH 6/8] Check if nodes parent is a member expression --- scripts/eslint-plugin/forward_ref_display_name.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/eslint-plugin/forward_ref_display_name.js b/scripts/eslint-plugin/forward_ref_display_name.js index b1c760448c0..22f2645be9e 100644 --- a/scripts/eslint-plugin/forward_ref_display_name.js +++ b/scripts/eslint-plugin/forward_ref_display_name.js @@ -46,9 +46,11 @@ module.exports = { } } }, - Identifier(node) { - if (node.name === 'displayName') { - displayNameUsages.push(node.parent.object); + MemberExpression(node) { + if (node.property.type === 'Identifier') { + if (node.name === 'displayName') { + displayNameUsages.push(node.parent.object); + } } }, 'Program:exit'() { From 1503957952785e67904bc5dcb427237eb37fc018 Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Wed, 13 May 2020 20:58:04 +0530 Subject: [PATCH 7/8] Updated cl --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d90cf72d9..43caa990280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## [`master`](https://github.com/elastic/eui/tree/master) -- Added eslint rule to detect `forwardRef` components without `displayName` ([#3451](https://github.com/elastic/eui/pull/3451)) +- Added `displayName` to components using `React.forwardRef` ([#3451](https://github.com/elastic/eui/pull/3451)) - Added `sortBy` and `sortShift` props to `euiPaletteColorBlind()` for sorting along the color wheel ([#3387](https://github.com/elastic/eui/pull/3387)) - Added `partition` key to `EuiChartThemeType` for Partition chart support ([#3387](https://github.com/elastic/eui/pull/3387)) - Updated `EuiImage`'s `caption` prop type from `string` to `ReactNode` ([#3387](https://github.com/elastic/eui/pull/3387)) From 9c4a536808b28df92bc1a50eaab83fe5ffed29bd Mon Sep 17 00:00:00 2001 From: Ashik Meerankutty Date: Fri, 15 May 2020 00:26:06 +0530 Subject: [PATCH 8/8] Fixed issue with rule --- scripts/eslint-plugin/forward_ref_display_name.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/eslint-plugin/forward_ref_display_name.js b/scripts/eslint-plugin/forward_ref_display_name.js index 22f2645be9e..918a4df95ab 100644 --- a/scripts/eslint-plugin/forward_ref_display_name.js +++ b/scripts/eslint-plugin/forward_ref_display_name.js @@ -47,10 +47,13 @@ module.exports = { } }, MemberExpression(node) { - if (node.property.type === 'Identifier') { - if (node.name === 'displayName') { - displayNameUsages.push(node.parent.object); - } + const { property } = node; + if ( + property && + property.type === 'Identifier' && + property.name === 'displayName' + ) { + displayNameUsages.push(node.object); } }, 'Program:exit'() {