[SIEM] Replace WithSource with useWithSource hook#68722
[SIEM] Replace WithSource with useWithSource hook#68722patrykkopycinski merged 13 commits intoelastic:masterfrom
Conversation
…r-with-source-hook # Conflicts: # x-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx
|
Pinging @elastic/siem (Team:SIEM) |
…r-with-source-hook # Conflicts: # x-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx
|
@elasticmachine merge upstream |
…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
|
@elasticmachine merge upstream |
XavierM
left a comment
There was a problem hiding this 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.
| ); | ||
|
|
||
| export const getBrowserFields = memoizeOne( | ||
| (_title: string, fields: IndexField[]): BrowserFields => |
There was a problem hiding this comment.
I think _title arguments was there to make the memoization simpler and faster than an object with a different reference
|
|
||
| export const useWithSource = (sourceId = 'default', indexToAdd?: string[] | null) => { | ||
| const [configIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY); | ||
| const [loading, updateLoading] = useState(false); |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| useEffect(() => { | ||
| const abortCtrl = new AbortController(); |
There was a problem hiding this comment.
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
| }, | ||
| (error) => { | ||
| updateLoading(false); | ||
| setState((prevState) => ({ ...prevState, errorMessage: error.message })); |
|
|
||
| return { indicesExist, browserFields, indexPattern, loading, errorMessage }; | ||
| async function fetchSource() { | ||
| updateLoading(true); |
There was a problem hiding this comment.
nit -> I think loading can belong to UseWithSourceState
XavierM
left a comment
There was a problem hiding this 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.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* 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) ...
* 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)
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
Refactor
WithSourcethat is using render props pattern touseWithSourcehooks based solution to make the code cleaner and avoid unnecessary re-renders.Checklist