[SIEM] Fix eslint errors from jsx-no-bind #1 #52856
Conversation
patrykkopycinski
added
Team:SIEM
v8.0.0
release_note:skip
patrykkopycinski
requested review from
XavierM,
FrankHassanabad,
spong and
andrew-goldstein
|
Pinging @elastic/siem (Team:SIEM) |
| @@ -15,7 +15,7 @@ import { SpyRoute } from '../../utils/route/spy_routes'; | |||
| import * as i18n from './translations'; | |||
|
|
|||
| const TimelinesContainer = styled.div` | |||
| width: 100%: | |||
| width: 100%; | |||
There was a problem hiding this comment.
Yeah, nice! We used to have a linter for styled components that was nice.
There was a problem hiding this comment.
I'm happy to add it back 😄
|
Are there before and after performance charts for this PR so we can see what the improvements are? |
x-pack/legacy/plugins/siem/public/components/page/hosts/hosts_table/index.tsx
Show resolved
Hide resolved
| @@ -93,7 +113,7 @@ export const NetworkDnsTableComponent = React.memo<NetworkDnsTableProps>( | |||
| } | |||
| } | |||
| }, | |||
| [sort, type] | |||
| [sort, type, tableType] | |||
There was a problem hiding this comment.
Why is tableType necessary here but not on updateLimitPagination & updateActivePage? I see it's excluded for most other callbacks as well.
Seems to be differing usages between here and the other tables onChange() as well -- I might be missing something here, but just want to verify what difference changes the consistency between tables.
There was a problem hiding this comment.
It was my mistake, the first round was done without any help, so I've added tableType to dependencies even though it was unnecessary, so to make sure that all dependencies are correct I've enabled temporarly react-hooks/exhaustive-deps rule and fixed everything as it suggested. I believe now it should be correct
| render: (value: TableData['activate'], item: TableData) => { | ||
| const handleRuleStateChange: RuleStateChangeCallback = async (enabled, id) => { | ||
| await enableRulesAction([id], enabled, dispatch, kbnVersion); | ||
| }; |
There was a problem hiding this comment.
Nice! Thanks for the cleanup here -- after reviewing this PR I'll be better prepared next time around 🙂
| to: fromTo.to, | ||
| }); | ||
| }} | ||
| narrowDateRange={narrowDateRange} |
x-pack/legacy/plugins/siem/public/components/page/network/users_table/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Checked out locally to test, and performed a code review. The app feels noticeably snappier -- thanks for combing through the codebase and making these optimizations @patrykkopycinski! As @FrankHassanabad mentioned, if you happen to have any perf charts of before/after, be sure to include them here for posterity.
For anyone passing through, be sure to check out the Perf Section from the Hooks FAQ for details on when and how you should declare dependencies, especially with regards to function dependencies.
Looking forward to Part II @patrykkopycinski, whereafter we can hopefully enable the jsx-no-bind linter rule for good 🙂
LGTM! 👍 🚀
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
I'm working on improving the performance of the SIEM app and one of the ways to avoid unnecessary re-renders is to memoize callbacks.
We have over 300 errors from jsx-no-bind rule, so I've decided to split fixes into couple PR, so they are more digestable ;)
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers