[Security Solution] Refactor useSelector #75297

patrykkopycinski · 2020-08-18T14:18:19Z

Summary

https://react-redux.js.org/api/hooks#equality-comparisons-and-updates

TL;DR: To keep the backward-compatibility with connect() while moving towards hooks we should use shallow comparison instead of the useSelector default, strict reference comparison.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…r-use-selector

stephmilovic · 2020-08-20T18:46:54Z

Can you please write a summary for this? Wondering the reasoning

patrykkopycinski · 2020-08-24T10:24:54Z

@elasticmachine merge upstream

stephmilovic · 2020-08-25T14:10:26Z

.eslintrc.js

+              {
+                name: 'react-redux',
+                importNames: ['useSelector'],
+                message: 'Please use "useShallowEqualSelector" instead or create your own selector',


you'll still get the error when you create your own selector correct?

I don't think we should add a rule here. Many teams work in this code base, for that reason I think we should all use the exact set of lint rules defined by Kibana core.

The Resolver code base, for example, has (AFAIK) no reason to use shallow comparison for selectors as it was designed with the expectation that strict comparison would be used.

stephmilovic · 2020-08-25T14:11:59Z

x-pack/plugins/security_solution/public/cases/components/use_all_cases_modal/index.tsx

 import { APP_ID } from '../../../../common/constants';
 import { SecurityPageName } from '../../../app/types';
 import { useKibana } from '../../../common/lib/kibana';
 import { getCaseDetailsUrl, getCreateCaseUrl } from '../../../common/components/link_to';
-import { State } from '../../../common/store';


I like that we don't have to import State everywhere now

stephmilovic

Code looks clean, performed manual testing around timeline. Couldn't find resolver data to test w today, but I'm confident from timeline testing to LGTM 🏁 🇵🇱

oatkiller

I am opposed to adding a lint rule.

oatkiller · 2020-08-25T16:17:25Z

.eslintrc.js

+              {
+                name: 'react-redux',
+                importNames: ['useSelector'],
+                message: 'Please use "useShallowEqualSelector" instead or create your own selector',


I don't think we should add a rule here. Many teams work in this code base, for that reason I think we should all use the exact set of lint rules defined by Kibana core.

The Resolver code base, for example, has (AFAIK) no reason to use shallow comparison for selectors as it was designed with the expectation that strict comparison would be used.

oatkiller

Would you please provide a change-by-change justification for the changes in Resolver. Resolver was designed with strict comparison of selectors in mind. We don't have plans to adopt the useShallowEqualSelector pattern.

oatkiller · 2020-08-25T16:28:52Z

x-pack/plugins/security_solution/public/resolver/view/panels/process_details.tsx

@@ -51,7 +52,7 @@ export const ProcessDetails = memo(function ProcessDetails({
 }) {
  const processName = event.eventName(processEvent);
  const entityId = event.entityId(processEvent);
-  const isProcessTerminated = useSelector(selectors.isProcessTerminated)(entityId);
+  const isProcessTerminated = useShallowEqualSelector(selectors.isProcessTerminated)(entityId);


Unrelated to this PR but: isProcessTerminated actually should be a parametric selector in order to work as expected.

oatkiller · 2020-08-25T16:30:09Z

x-pack/plugins/security_solution/public/resolver/view/panels/related_event_detail.tsx

-  const relatedEventsForThisProcess = useSelector(selectors.relatedEventsByEntityId).get(
-    processEntityId!
-  );
+  const relatedEventsForThisProcess = useShallowEqualSelector(


Also unrelated to this PR, but I'm noticing that in some cases we return a memoized thunk from a selector but we actually expect the return value of the thunk to trigger a rerender. In these cases we should consider a parametric selector (still using strict comparison, as all Resolver selectors return primitives or memoized values.)

oatkiller · 2020-08-25T17:41:14Z

Suggestion on this PR:

Open a PR that introduces this hook and uses it in places where it's known to work (and solve an issue)
Communicate with the other teams (e.g. via an Issue or email) your idea for standardizing on this. Take feedback.
Revisit this PR.

patrykkopycinski · 2020-08-25T18:29:40Z

@oatkiller you're right, I've reverted all changes related to /management/ and /resolver/ part. Reverted also changes to the ESLint config

Fixed my issues.

oatkiller · 2020-08-25T18:39:31Z

@patrykkopycinski Thank you very much. Your PR brought to our attention some potential bugs in Resolver, I thank you for that. Also, we will keep in mind the hook you added and consider using it in the future.

patrykkopycinski · 2020-08-31T06:22:22Z

@elasticmachine merge upstream

patrykkopycinski · 2020-08-31T08:55:39Z

@elasticmachine merge upstream

patrykkopycinski · 2020-08-31T20:26:35Z

@elasticmachine merge upstream

…r-use-selector # Conflicts: # x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.test.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/body/events/stateful_event.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/insert_timeline_popover/index.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/properties/use_create_timeline.tsx

kibanamachine · 2020-09-28T12:16:08Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: a0f0700
Flaky suites:
- xpack-securitySolutionCypress

Test Failures

Mark one alert in progress when more than one open alerts are selected.Alerts Marking alerts as in-progress Mark one alert in progress when more than one open alerts are selected

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has failed 1 times on tracked branches: https://github.com/elastic/kibana/issues/78557

AssertionError: Timed out retrying: expected '<span.euiBadge__text>' to have text '1', but the text was '107'
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/alerts.spec.ts:726:51)

Metrics [docs]

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	1969	+1	1968

async chunks size

id	value	diff	baseline
securitySolution	10.2MB	+3.5KB	10.2MB

page load bundle size

id	value	diff	baseline
securitySolution	581.0KB	+1.0B	581.0KB

History

💚 Build #71192 succeeded 73ead59
💔 Build #70443 failed c46389d
💔 Build #70353 failed bfdf64c
💚 Build #70142 succeeded 437c6ef
💔 Build #69938 failed 1ea3502

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

@timestamp

* master: (226 commits) [Enterprise Search] Added Logic for the Credentials View (elastic#77626) [CSM] Js errors (elastic#77919) Add the @kbn/apm-config-loader package (elastic#77855) [Security Solution] Refactor useSelector (elastic#75297) Implement tagcloud renderer (elastic#77910) [APM] Alerting: Add global option to create all alert types (elastic#78151) [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939) TypeScript cleanup in visualizations plugin (elastic#78428) Lazy load metric & mardown visualizations (elastic#78391) [Detections][EQL] EQL rule execution in detection engine (elastic#77419) Update tutorial-full-experience.asciidoc (elastic#75836) Update tutorial-define-index.asciidoc (elastic#75754) Add support for runtime field types to mappings editor. (elastic#77420) [Monitoring] Usage collection (elastic#75878) [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316) [Security Solution][Resolver] Update @timestamp formatting (elastic#78166) [Security Solution] Fix app layout (elastic#76668) [Security Solution][Resolver] 2 new functions to DAL (elastic#78477) Adds new elasticsearch client to telemetry plugin (elastic#78046) skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500) ...

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

[Security Solution] Refactor useSelector

6e840b7

patrykkopycinski added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 18, 2020

patrykkopycinski self-assigned this Aug 18, 2020

patrykkopycinski added 3 commits August 18, 2020 22:40

Fix types

186d4e6

Merge branch 'master' of github.com:elastic/kibana into chore/refacto…

a1d4462

…r-use-selector

add useDeepEqualSelector

865367a

patrykkopycinski marked this pull request as ready for review August 19, 2020 10:13

patrykkopycinski requested review from a team as code owners August 19, 2020 10:13

elasticmachine and others added 2 commits August 24, 2020 04:25

Merge branch 'master' into chore/refactor-use-selector

1ea3502

fix lint

437c6ef

stephmilovic reviewed Aug 25, 2020

View reviewed changes

stephmilovic approved these changes Aug 25, 2020

View reviewed changes

oatkiller suggested changes Aug 25, 2020

View reviewed changes

oatkiller previously requested changes Aug 25, 2020

View reviewed changes

oatkiller reviewed Aug 25, 2020

View reviewed changes

patrykkopycinski added 2 commits August 25, 2020 20:26

revert changes in resolver

ce0c6bc

leftovers

bfdf64c

Update use_selector.tsx

c46389d

Merge branch 'master' into chore/refactor-use-selector

73ead59

patrykkopycinski merged commit 2defe88 into elastic:master Sep 28, 2020

patrykkopycinski deleted the chore/refactor-use-selector branch September 28, 2020 13:28

patrykkopycinski mentioned this pull request Sep 28, 2020

[7.x] [Security Solution] Refactor useSelector (#75297) #78584

Merged

patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Sep 28, 2020

[Security Solution] Refactor useSelector (elastic#75297)

ad37f02

patrykkopycinski added a commit that referenced this pull request Sep 28, 2020

[Security Solution] Refactor useSelector (#75297) (#78584)

943fc14

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Refactor useSelector #75297

[Security Solution] Refactor useSelector #75297

patrykkopycinski commented Aug 18, 2020 •

edited

Loading

stephmilovic commented Aug 20, 2020

patrykkopycinski commented Aug 24, 2020

stephmilovic Aug 25, 2020

patrykkopycinski Aug 25, 2020

oatkiller Aug 25, 2020 •

edited

Loading

stephmilovic Aug 25, 2020

stephmilovic left a comment

oatkiller left a comment

oatkiller Aug 25, 2020 •

edited

Loading

oatkiller left a comment •

edited

Loading

oatkiller Aug 25, 2020

oatkiller Aug 25, 2020 •

edited

Loading

oatkiller commented Aug 25, 2020

patrykkopycinski commented Aug 25, 2020

oatkiller commented Aug 25, 2020

patrykkopycinski commented Aug 31, 2020

patrykkopycinski commented Aug 31, 2020

patrykkopycinski commented Aug 31, 2020

kibanamachine commented Sep 28, 2020

Stack Trace

[Security Solution] Refactor useSelector #75297

[Security Solution] Refactor useSelector #75297

Conversation

patrykkopycinski commented Aug 18, 2020 • edited Loading

Summary

Checklist

stephmilovic commented Aug 20, 2020

patrykkopycinski commented Aug 24, 2020

stephmilovic Aug 25, 2020

Choose a reason for hiding this comment

patrykkopycinski Aug 25, 2020

Choose a reason for hiding this comment

oatkiller Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

stephmilovic Aug 25, 2020

Choose a reason for hiding this comment

stephmilovic left a comment

Choose a reason for hiding this comment

oatkiller left a comment

Choose a reason for hiding this comment

oatkiller Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

oatkiller left a comment • edited Loading

Choose a reason for hiding this comment

oatkiller Aug 25, 2020

Choose a reason for hiding this comment

oatkiller Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

oatkiller commented Aug 25, 2020

patrykkopycinski commented Aug 25, 2020

oatkiller commented Aug 25, 2020

patrykkopycinski commented Aug 31, 2020

patrykkopycinski commented Aug 31, 2020

patrykkopycinski commented Aug 31, 2020

kibanamachine commented Sep 28, 2020

💛 Build succeeded, but was flaky

Test Failures

Stack Trace

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

patrykkopycinski commented Aug 18, 2020 •

edited

Loading

oatkiller Aug 25, 2020 •

edited

Loading

oatkiller Aug 25, 2020 •

edited

Loading

oatkiller left a comment •

edited

Loading

oatkiller Aug 25, 2020 •

edited

Loading