[SIEM] Add react/display-name eslint rule by patrykkopycinski · Pull Request #53107 · elastic/kibana

patrykkopycinski · 2019-12-16T10:51:34Z

Summary

Add eslint rule to make sure we always add displayName to the components

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-16T10:52:19Z

Pinging @elastic/siem (Team:SIEM)

elasticmachine · 2019-12-16T12:03:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2f04edc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…lint-display-name # Conflicts: # x-pack/legacy/plugins/siem/public/apps/start_app.tsx # x-pack/legacy/plugins/siem/public/components/page/hosts/uncommon_process_table/index.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/components/activity_monitor/columns.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/rule_details/index.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/columns.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/signals/components/closed_signals/index.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/signals/components/open_signals/index.tsx # x-pack/legacy/plugins/siem/public/pages/detection_engine/signals/index.tsx

spong · 2020-01-06T18:48:05Z

@elasticmachine merge upstream

spong · 2020-01-06T21:37:10Z

x-pack/legacy/plugins/siem/public/apps/start_app.tsx

 * you may not use this file except in compliance with the Elastic License.
 */

+/* eslint-disable react/display-name */


May want to add a comment here as to why this is being disabled -- when I added displayName's for the below three components (AppPluginRoot, StartApp, and SiemApp) they were not being shown in the react dev tools Component tree, so I assume this is the reason?

spong · 2020-01-06T21:48:15Z

x-pack/legacy/plugins/siem/public/components/alerts_viewer/alerts_table.tsx

+  pageFilters?: esFilters.Filter[];
+}
+
+const AlertsTableComponent: React.FC<Props> = ({ endDate, startDate, pageFilters = [] }) => {


What is the intent of creating this intermediary AlertsTableComponent using React.FC and then wrapping as AlertsTable using React.memo vs the previous implementation of just using React.memo?

React.memo propagates name of the wrapped component, but as we were passing an inline function to the memo we end up with Anonymus (Memo)

Ahhh, perfect, that's what I was wondering! 🙂

For anyone passing through, @patrykkopycinski outlines this behavior in https://github.com/elastic/siem-team/issues/485.

spong · 2020-01-06T22:03:11Z

x-pack/legacy/plugins/siem/public/components/page/hosts/authentications_table/index.tsx

+/* eslint-disable react/display-name */
+


Just noting the consistency of naming our connected components. Seems it's hit or miss on if we set the displayName after calling connect(). Here in AuthenticationTable we do not set displayName after connecting, but we do for UncommonProcessTable. The linter rule does not enforce it for connected components, so we'll need to keep an eye out in reviews (or perhaps see if the updated react-redux can handle setting the displayName?).

with react-redux@7 we will be able to migrate from connect to hooks, so the linter will be able to properly enforce the rule 💪

spong · 2020-01-06T22:17:08Z

...plugins/siem/public/pages/detection_engine/components/signals/signals_filter_group/index.tsx

+  onFilterGroupChanged: (filterGroup: SignalFilterOption) => void;
+}
+
+const SignalsTableFilterGroupComponent: React.FC<Props> = ({ onFilterGroupChanged }) => {


As above, why the change to React.FC and then wrapping in React.memo below?

Also, in this instance, there is no displayName set for either SignalsTableFilterGroupComponent or SignalsTableFilterGroup and I'm seeing no linter error. If I move SignalsTableFilterGroupComponent back to React.memo I then see the "Component definition is missing displayName" linter error. Any idea why we're not seeing errors here?

for components defined with const the variable name is propagated automatically as a component name, so we don't have to do it manually

spong · 2020-01-06T22:20:26Z

.../plugins/siem/public/pages/detection_engine/components/signals/signals_utility_bar/index.tsx

-    sendSignalsToTimeline,
-  }) => {
-    const [defaultNumberFormat] = useUiSetting$<string>(DEFAULT_NUMBER_FORMAT);
+const SignalsUtilityBarComponent: React.FC<SignalsUtilityBarProps> = ({


Same clarification as above -- why React.FC then wrapped with React.memo? Also, both SignalsUtilityBarComponent and SignalsUtilityBar are missing displayName and no linter error is shown.

spong · 2020-01-06T22:28:51Z

x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/step_panel/index.tsx

+
+export const StepPanel = memo(StepPanelComponent);
+
+StepPanel.displayName = 'StepPanel';


Seems react-dev-tools will detect the memo and use the displayName of the component being wrapped (adding a Memo tag), so we don't need to add a display name for subsequent memo's (unlike the behavior when wrapping components with connect()).

spong · 2020-01-06T22:33:37Z

x-pack/legacy/plugins/siem/public/pages/detection_engine/components/signals_chart/index.tsx

+
+SignalsChartsComponent.displayName = 'SignalsChartsComponent';
+
+export const SignalsCharts = memo(SignalsChartsComponent);


Same question here for why we're abstracting the memo to a parent component? (This component has no props either fwiw)

spong

Tested locally and performed code review. These changes look good to me, but I left a few comments around our methods of adding displayName to connected and memoized components. Approving now, but we may want to standardize on these methods/add additional comments so any inconsistencies don't propagate through the codebase.

…lint-display-name

kibanamachine · 2020-01-07T12:37:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c2d48f2

History

💚 Build #18307 succeeded a295ce0
💚 Build #18167 succeeded 6321638
💚 Build #17951 succeeded 1385da5
💔 Build #17941 failed bc1f53d
💚 Build #15689 succeeded 2f04edc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

patrykkopycinski · 2020-01-07T12:51:06Z

@spong Thank you for your comments, I did another round and now I believe it was fixed properly. Would you mind to take a look again?

spong

Reviewed latest changes and LGTM! Thanks for the additional details and fixes @patrykkopycinski!

For anyone passing through, please be sure to check out the following two links for details on setting dispayName (and why creating a FC and wrapping via memo is necessary):

…le-saved-objects * 'master' of github.com:elastic/kibana: (86 commits) [Uptime] Added date range filter into expanded list query (elastic#52609) [SIEM] Add react/display-name eslint rule (elastic#53107) [SIEM] Enable eslint prefer-template rule (elastic#53983) Elasticsearch snapshots automation (elastic#53706) [SIEM] Enable eslint react/no-children-prop (elastic#53985) [DOCS][Reporting] Adds info about using a custom reporting index (elastic#54052) Changing background color to align with EUI color (elastic#54060) Fix linting issues (elastic#54068) NP Migration: Move doc views registry and existing doc views into discover plugin (elastic#53465) [ML] DF Analytics job creation: Add 'excludes' input field to form (elastic#53856) EMT-issue-65: add endpoint list api (elastic#53861) [SIEM] Fix doubled drag handles in Timeline (elastic#52679) Functional tests: refactor visualize_page (elastic#53845) [Dashboard] Redesign empty screen in readonly mode (elastic#54073) [ML] Support search for partitions on Single Metric Viewer (elastic#53879) update apm index pattern (elastic#54095) Add data ui for envoyproxy Metricbeat Module (elastic#53476) [ML] persist the brush when expanded to full width (elastic#54020) Skip failing test (elastic#54100) [APM] Show errors on the timeline instead of under the transaction (elastic#53756) ... # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/contexts/services_context.tsx # src/legacy/core_plugins/console/public/np_ready/application/index.tsx

…/kibana into feature/console-saved-objects * 'feature/console-saved-objects' of github.com:jloleysens/kibana: (86 commits) [Uptime] Added date range filter into expanded list query (elastic#52609) [SIEM] Add react/display-name eslint rule (elastic#53107) [SIEM] Enable eslint prefer-template rule (elastic#53983) Elasticsearch snapshots automation (elastic#53706) [SIEM] Enable eslint react/no-children-prop (elastic#53985) [DOCS][Reporting] Adds info about using a custom reporting index (elastic#54052) Changing background color to align with EUI color (elastic#54060) Fix linting issues (elastic#54068) NP Migration: Move doc views registry and existing doc views into discover plugin (elastic#53465) [ML] DF Analytics job creation: Add 'excludes' input field to form (elastic#53856) EMT-issue-65: add endpoint list api (elastic#53861) [SIEM] Fix doubled drag handles in Timeline (elastic#52679) Functional tests: refactor visualize_page (elastic#53845) [Dashboard] Redesign empty screen in readonly mode (elastic#54073) [ML] Support search for partitions on Single Metric Viewer (elastic#53879) update apm index pattern (elastic#54095) Add data ui for envoyproxy Metricbeat Module (elastic#53476) [ML] persist the brush when expanded to full width (elastic#54020) Skip failing test (elastic#54100) [APM] Show errors on the timeline instead of under the transaction (elastic#53756) ...

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (55 commits) [ui/public/utils] Copy rarely used items to where they are consumed (elastic#53819) set AppArch team as an owner of the search endpoints (elastic#54131) Don't expose Elasticsearch client as Observable (elastic#53824) [SIEM] Cleanup unnecessary use of enzyme-to-json (elastic#53980) fix ui exports doc (elastic#54138) change markdown element title (elastic#54194) [Logs UI] Refactor log position to hooks (elastic#53540) [SIEM] Implement NP Plugin Setup (elastic#54030) [DOCS] Updates ML links (elastic#53613) sort renovate packages in config Spaces - fix flakey api tests (elastic#54154) Remove dependency that was causing effect to re-execute infinitely. (elastic#54160) [dev/run] expose unexpected flags as more than just names (elastic#54080) [DOCS] Moves index pattern doc to Discover (elastic#53347) [SIEM] Cleanup React imports (elastic#53981) Update eslint related packages (elastic#54107) [Uptime] Added date range filter into expanded list query (elastic#52609) [SIEM] Add react/display-name eslint rule (elastic#53107) [SIEM] Enable eslint prefer-template rule (elastic#53983) Elasticsearch snapshots automation (elastic#53706) ...

[SIEM] Add react/display-name eslint rule

2f04edc

patrykkopycinski self-assigned this Dec 16, 2019

patrykkopycinski added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 Team:SIEM labels Dec 16, 2019

patrykkopycinski requested review from FrankHassanabad, XavierM and spong December 17, 2019 22:48

patrykkopycinski requested review from andrew-goldstein, angorayc, rylnd and stephmilovic January 4, 2020 16:15

patrykkopycinski added 3 commits January 4, 2020 17:30

cleanup

bc1f53d

cleanup

1385da5

Merge branch 'master' into chore/siem-eslint-display-name

6321638

spong reviewed Jan 6, 2020

View reviewed changes

spong approved these changes Jan 6, 2020

View reviewed changes

patrykkopycinski added 2 commits January 7, 2020 10:07

Merge branch 'master' of github.com:elastic/kibana into chore/siem-es…

a295ce0

…lint-display-name

fix properly displayNames

c2d48f2

spong approved these changes Jan 7, 2020

View reviewed changes

patrykkopycinski merged commit 23a0513 into elastic:master Jan 7, 2020

patrykkopycinski deleted the chore/siem-eslint-display-name branch January 7, 2020 17:05

patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Jan 7, 2020

[SIEM] Add react/display-name eslint rule (elastic#53107)

4889510

patrykkopycinski mentioned this pull request Jan 7, 2020

[7.x] [SIEM] Add react/display-name eslint rule (#53107) #54159

Merged

rylnd pushed a commit that referenced this pull request Jan 7, 2020

[SIEM] Add react/display-name eslint rule (#53107) (#54159)

81ef6e0

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>


		export const StepPanel = memo(StepPanelComponent);

		StepPanel.displayName = 'StepPanel';


		SignalsChartsComponent.displayName = 'SignalsChartsComponent';

		export const SignalsCharts = memo(SignalsChartsComponent);

Conversation

patrykkopycinski commented Dec 16, 2019

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 16, 2019

Uh oh!

elasticmachine commented Dec 16, 2019

💚 Build Succeeded

Uh oh!

spong commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spong left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 7, 2020

💚 Build Succeeded

History

Uh oh!

patrykkopycinski commented Jan 7, 2020

Uh oh!

spong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants