New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SIEM] Replace WithSource with useWithSource hook #68722

Merged

patrykkopycinski merged 13 commits into elastic:master from patrykkopycinski:chore/refactor-with-source-hook

Jun 25, 2020

Contributor

patrykkopycinski commented Jun 9, 2020 •

edited

Loading

Summary

Refactor WithSource that is using render props pattern to useWithSource hooks based solution to make the code cleaner and avoid unnecessary re-renders.

Checklist

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


          [SIEM] Replace WithSource with useWithSoure hook

21e7d23

patrykkopycinski added Team:SIEM v8.0.0 release_note:skip v7.9.0 labels

patrykkopycinski self-assigned this


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

749507d

…r-with-source-hook

# Conflicts:
#	x-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx

patrykkopycinski marked this pull request as ready for review

June 10, 2020 22:39

patrykkopycinski requested review from a team as code owners

June 10, 2020 22:39

Contributor

elasticmachine commented Jun 10, 2020

Pinging @elastic/siem (Team:SIEM)


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

e165a6d

…r-with-source-hook

# Conflicts:
#	x-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx

Contributor Author

patrykkopycinski commented Jun 16, 2020

@elasticmachine merge upstream

elasticmachine and others added 3 commits

June 16, 2020 11:30


          Merge branch 'master' into chore/refactor-with-source-hook

d76a91f


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

978c8a7

…r-with-source-hook

# Conflicts:
#	x-pack/plugins/security_solution/public/alerts/pages/detection_engine/detection_engine.test.tsx
#	x-pack/plugins/security_solution/public/alerts/pages/detection_engine/detection_engine.tsx
#	x-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx
#	x-pack/plugins/security_solution/public/app/home/index.tsx
#	x-pack/plugins/security_solution/public/common/components/header_global/__snapshots__/index.test.tsx.snap
#	x-pack/plugins/security_solution/public/common/components/header_global/index.test.tsx
#	x-pack/plugins/security_solution/public/common/components/header_global/index.tsx
#	x-pack/plugins/security_solution/public/network/pages/ip_details/__snapshots__/index.test.tsx.snap
#	x-pack/plugins/security_solution/public/network/pages/ip_details/index.test.tsx
#	x-pack/plugins/security_solution/public/network/pages/ip_details/index.tsx
#	x-pack/plugins/security_solution/public/overview/pages/overview.tsx


          cleanup

ea0e726

Contributor

XavierM commented Jun 24, 2020

@elasticmachine merge upstream


          Merge branch 'master' into chore/refactor-with-source-hook

c126b16

XavierM reviewed

View reviewed changes

Contributor

XavierM left a comment

Let's talk about my two comments, I am not 100% sure anymore about my first comment but I am pretty sure that's why we did it like that but it might not be valid anymore.

After the implementation looks good to me, I will fully test and ask someone in the alert team to test it to make sure we are not missing something over there.

x-pack/plugins/security_solution/public/common/containers/source/index.tsx Outdated

@@ @@ -75,7 +62,7 @@ export const getIndexFields = memoizeOne( @@
               );
               export const getBrowserFields = memoizeOne(
-                (_title: string, fields: IndexField[]): BrowserFields =>

Contributor

XavierM Jun 24, 2020

I think _title arguments was there to make the memoization simpler and faster than an object with a different reference

x-pack/plugins/security_solution/public/common/containers/source/index.tsx Outdated

		const [configIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
		const [loading, updateLoading] = useState(false);

Contributor

XavierM Jun 24, 2020

something that we learned, it is better to create a complex state object than multiple states. With only one state, we will avoid a lot of re-render. We discover that in the detection development and we refactor most of api hooks to manage with one state.

patrykkopycinski added 2 commits

June 24, 2020 19:45


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

1b78891

…r-with-source-hook


          PR comments

e8bf05f

XavierM reviewed

View reviewed changes

x-pack/plugins/security_solution/public/common/containers/source/index.tsx

-                        }
-                      );
-                  }
-                }
                 useEffect(() => {
                   const abortCtrl = new AbortController();

Contributor

XavierM Jun 25, 2020

I think we are missing something fundamental here, to avoid to update the state if the promise has been canceled.

facebook/react#14326 (comment) or like here in our code https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/alerts/containers/detection_engine/alerts/use_privilege_user.tsx#L40

XavierM reviewed

View reviewed changes

x-pack/plugins/security_solution/public/common/containers/source/index.tsx Outdated

+                          },
+                          (error) => {
+                            updateLoading(false);
+                            setState((prevState) => ({ ...prevState, errorMessage: error.message }));

Contributor

XavierM Jun 25, 2020

Nice!

XavierM reviewed

View reviewed changes

x-pack/plugins/security_solution/public/common/containers/source/index.tsx Outdated

-                return { indicesExist, browserFields, indexPattern, loading, errorMessage };
+                  async function fetchSource() {
+                    updateLoading(true);

Contributor

XavierM Jun 25, 2020

nit -> I think loading can belong to UseWithSourceState

XavierM approved these changes

View reviewed changes

Contributor

XavierM left a comment

I tested locally and I did not see issues with our browser fields and/or KQL bar. I really do like what you did for us.

Please make sure to resolve https://github.com/elastic/kibana/pull/68722/files#r445265906 before merging.

patrykkopycinski added 4 commits

June 25, 2020 08:28


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

11cad3c

…r-with-source-hook


          PR comments

fb07f14


          PR comments

96dcd74


          memoize value

cd8aac8

Contributor

kibanamachine commented Jun 25, 2020

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💚 Build #56225 succeeded 96dcd74
💚 Build #56186 succeeded fb07f14
💚 Build #56061 succeeded e8bf05f
💚 Build #55779 succeeded c126b16
💚 Build #55389 succeeded ea0e726

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

patrykkopycinski merged commit ef496ff into elastic:master

patrykkopycinski deleted the chore/refactor-with-source-hook branch

June 25, 2020 16:08

patrykkopycinski mentioned this pull request

[7.x] [SIEM] Replace WithSource with useWithSource hook (#68722) #69974

Merged

patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request


          [SIEM] Replace WithSource with useWithSource hook (elastic#68722)

a220883

gmmorris added a commit to gmmorris/kibana that referenced this pull request


          Merge branch 'master' into alerting/migrate-old-alerting-consumer

045c68e

* master: (90 commits)
  [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513)
  [SIEM] Replace WithSource with useWithSource hook (elastic#68722)
  [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708)
  rename old siem kibana config to securitySolution (elastic#69874)
  Remove unused Resolver code (elastic#69914)
  [Observability] Fixing dynamic return type based on the appName (elastic#69894)
  [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419)
  Fixes special clicks and 3rd party icon sizes in nav (elastic#69767)
  [APM] Catch annotations index permission error and log warning (elastic#69881)
  [Endpoint][Ingest Manager] minor code cleanup (elastic#69844)
  [Logs UI] Logs ui context menu (elastic#69915)
  Index pattern serialize and de-serialize (elastic#68844)
  [QA] Unskip functional tests (elastic#69760)
  [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715)
  Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863)
  PR: Provide limit warnings to user when API limits are reached. (elastic#69590)
  [Maps] Remove broken button (elastic#69853)
  Makes usage collection methods available on start (elastic#69836)
  [SIEM][CASE] Improve Jira's labelling (elastic#69892)
  [Logs UI] Access ML via the programmatic plugin API (elastic#68905)
  ...

patrykkopycinski added a commit that referenced this pull request


          [SIEM] Replace WithSource with useWithSource hook (#68722) (#69974)

9bac010

gmmorris added a commit to gmmorris/kibana that referenced this pull request


          Merge branch 'master' into alerting/consumer-based-rbac

2e75199

* master:
  [Encrypted Saved Objects] Adds support for migrations in ESO (elastic#69513)
  [SIEM] Replace WithSource with useWithSource hook (elastic#68722)
  [Endpoint]EMT-451: add ability to filter endpoint metadata based on presence of unenrolled events (elastic#69708)
  rename old siem kibana config to securitySolution (elastic#69874)
  Remove unused Resolver code (elastic#69914)
  [Observability] Fixing dynamic return type based on the appName (elastic#69894)
  [SECURITY SOLUTION][INGEST] Task/endpoint list tests (elastic#69419)
  Fixes special clicks and 3rd party icon sizes in nav (elastic#69767)
  [APM] Catch annotations index permission error and log warning (elastic#69881)
  [Endpoint][Ingest Manager] minor code cleanup (elastic#69844)
  [Logs UI] Logs ui context menu (elastic#69915)
  Index pattern serialize and de-serialize (elastic#68844)

MindyRS added the Team: SecuritySolution label

Contributor

elasticmachine commented Sep 23, 2021

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Team: SecuritySolution Team:SIEM v7.9.0 v8.0.0