Skip to content
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.

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

[security_solution] enable react-hooks/exhaustive-deps #68470

Conversation

oatkiller
Copy link
Contributor

Summary

  • Enable react-hooks/exhaustive-deps rule for security_solution
  • Disable it anywhere that it would catch an issue

Checklist

Delete any items that are not applicable to this PR.

For maintainers

* Enable `react-hooks/exhaustive-deps` rule for security_solution
* Disable it anywhere that it would catch an issue
@oatkiller oatkiller requested review from a team as code owners June 6, 2020 12:41
@@ -46,7 +46,7 @@ export const useRules = ({
let isSubscribed = true;
const abortCtrl = new AbortController();

async function fetchData(forceReload: boolean = false) {
async function fetchData() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing unused parameters

@@ -80,7 +80,7 @@ export const useRules = ({

fetchData();
reFetchRules.current = (refreshPrePackagedRule: boolean = false) => {
fetchData(true);
fetchData();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing unused parameters

}, [
pagination.page,
pagination.perPage,
filterOptions.filter,
filterOptions.sortField,
filterOptions.sortOrder,
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor Author

@oatkiller oatkiller Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a sort on every render is not generally advisable. Even if it happened to be harmless here, when reading this code I feel the urge to confirm that filterOptions.tags is of a constant size.

Might be worth scheduling work to improve. We could probably just wrap it in a useMemo if nothing else.

@@ -91,8 +91,10 @@ export const Create = React.memo(() => {
const onSubmit = useCallback(async () => {
const { isValid, data } = await form.submit();
if (isValid) {
// `postCase`'s type is incorrect, it actually returns a promise
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment correct? I think we could update the type.

@@ -50,7 +50,7 @@ const FlyoutPaneComponent: React.FC<FlyoutPaneComponentProps> = ({
const dispatch = useDispatch();

const onResizeStop: ResizeCallback = useCallback(
(e, direction, ref, delta) => {
(_e, _direction, _ref, delta) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell tsserver/tsc to ignore the fact that these are unused

await deleteTimelines(timelineIds);
},
[deleteTimelines]
);

/** Invoked when the user clicks the action to delete the selected timelines */
const onDeleteSelected: OnDeleteSelected = useCallback(async () => {
// The type for `deleteTimelines` is incorrect, it returns a Promise
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment right? We could probably fix the type.

influencersOrCriteriaToString(criteriaFields),
startDate,
endDate,
skip,
userPermissions,
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth improving this, as sorting something each render could hinder performance.

@@ -46,6 +46,7 @@ export const GroupsFilterPopoverComponent = ({

useEffect(() => {
onSelectedGroupsChanged(selectedGroups);
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another instance of sorting in render. Thoughts on these?

}, [
featureIndex,
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another sort in render

@oatkiller oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 6, 2020
@oatkiller oatkiller mentioned this pull request Jun 7, 2020
2 tasks
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@oatkiller oatkiller merged commit bc39186 into elastic:master Jun 8, 2020
@oatkiller oatkiller deleted the enable-exhaustive-deps-on-security-solution branch June 8, 2020 14:43
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jun 8, 2020
* Enable `react-hooks/exhaustive-deps` rule for security_solution
* Disable it anywhere that it would catch an issue
# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/exceptions/viewer/exception_entries.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 8, 2020
* master:
  [ML] DFAnalytics results: ensure ml result fields are shown in data grid (elastic#68305)
  [security_solution] enable react-hooks/exhaustive-deps (elastic#68470)
  Closes elastic#66867 by adding missing, requried API params (elastic#68465)
  [Telemetry] collect number of visualization saved in the past 7, 30 and 90 days (elastic#67865)
  [Logs UI] View in context tweaks (elastic#67777)
  Kibana developer examples landing page (elastic#67049)
  Bump decompress package version (elastic#68386)
  fix elastic#66185 (elastic#66186)
  Bump pdfmake package version (elastic#68395)
  Unskip embeddables/adding_children suite (elastic#68111)
  Add embed mode options in the Share UI (elastic#58435)
  Adding key to avoid react warning (elastic#68491)
oatkiller pushed a commit that referenced this pull request Jun 9, 2020
* Enable `react-hooks/exhaustive-deps` rule for security_solution
* Disable it anywhere that it would catch an issue
# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/exceptions/viewer/exception_entries.tsx
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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 Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants